Skip to content

Conversation

Ladas552
Copy link

This PR aims to make formatting of avante.nvim more consistent.
Adding periods to descriptions, fixing missed capitalization, transforming multiline string to one-liners, instead of merging default = true;, use mkOption.

I don't know if or how I should add it to changelog, because it doesn't affect the end user, but only future maintainers of said module. Per your request and suggestion, I would commit them too.

Sanity Checking

  • I have updated the changelog as per my changes
  • I have tested, and self-reviewed my code
  • My changes fit guidelines found in hacking nvf
  • Style and consistency
    • I ran Alejandra to format my code (nix fmt)
    • My code conforms to the editorconfig configuration of the project
    • My changes are consistent with the rest of the codebase
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas
    • I have added a section in the manual
    • (For breaking changes) I have included a migration guide
  • Package(s) built:
    • .#nix (default package)
    • .#maximal
    • .#docs-html (manual, must build)
    • .#docs-linkcheck (optional, please build if adding links)
  • Tested on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin

Add a 👍 reaction to pull requests you find important.

@Ladas552 Ladas552 changed the title avante-nvim: reduce mkEnableOption usage and descriptions avante-nvim: reduce mkEnableOption usage and reformat descriptions Aug 10, 2025
github-actions bot pushed a commit that referenced this pull request Aug 10, 2025
Copy link

github-actions bot commented Aug 10, 2025

🚀 Live preview deployed from 1681ad7

View it here:

Debug Information

Triggered by: Ladas552

HEAD at: avante.nvim-format

Reruns: 1202

Copy link
Owner

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, great change overall. I'm not too opposed to mkEnableOption but we can benefit from more fine-grained control without the merge operators as long as it's consistent with the codebase and documentation.

Left a few comments.

Comment on lines 114 to 118
auto_set_highlight_group = mkOption {
type = bool;
default = true;
description = "Automatically set the highlight group for the current line.";
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think boolean options should use mkEnableOption because of the "Whether to..." prefix. If replacing (which is fine) please consider "Whether to..." at the beginning so that it's more obvious this is a boolean logical op.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still confused on whether to use mkOption, and format description to fit into mkEnableOption standardized strings. Or would merging the "default = true;", as in the original version of the module, be better.

The latter looks ugly, but could be refactored later easily if you ever want to add mkEnableTrue or similar alias to the mkOption.

I can revert mkOption added in this PR back to mkEnableOption, and provide better fitting descriptions if need be

github-actions bot pushed a commit that referenced this pull request Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants