Skip to content

zsh: rename option syntax-highlighting#21

Merged
fpletz merged 1 commit intomasterfrom
rename-zsh-highlighting-option
May 11, 2017
Merged

zsh: rename option syntax-highlighting#21
fpletz merged 1 commit intomasterfrom
rename-zsh-highlighting-option

Conversation

@WilliButz
Copy link
Collaborator

rename zsh-option "syntax-highlighting" -> "syntaxHighlighting"

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Copy link

Choose a reason for hiding this comment

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

you have to update the programs.zsh.syntax-highlighting.patterns option as well...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I obviously missed that 👍

@Ma27
Copy link

Ma27 commented May 11, 2017

as I'm the author of this module: do we have any consistent style guide which describes the notation of module names? I've seen several modules in the module-list using dash and underscore notation, so it seems as camelCase is not the only thing in use.

rename zsh-option "syntax-highlighting" -> "syntaxHighlighting"
@WilliButz WilliButz force-pushed the rename-zsh-highlighting-option branch from 9fa3664 to 4a24282 Compare May 11, 2017 16:06
@WilliButz
Copy link
Collaborator Author

@Ma27 I don't know of any consistent style guide. My intention was to make the zsh options more similar.
Most of them are enableSomeOption and I thought why is not enableSyntaxHighlighting.

When I saw that there are more syntax-related options than just enable, I renamed it to something as close to the others as possible. :)

@fpletz
Copy link

fpletz commented May 11, 2017

I didn't find any documentation on the case of NixOS option names but most core options (not regarding services/programs) seem to be camel-case.

@fpletz
Copy link

fpletz commented May 11, 2017

@Ma27 If that's okay with you, I would merge this and push to upstream.

@groxxda
Copy link

groxxda commented May 11, 2017

http://nixos.org/nixpkgs/manual/#sec-syntax :

Use lowerCamelCase for variable names [...]

@Ma27
Copy link

Ma27 commented May 11, 2017

@groxxda thanks for pointing that out.

@fpletz @WilliButz I'm fine with that. When I built that some weeks ago I was unsure which notation to take, so I decided to stay consistent with the package name.
However please keep in mind that oh-my-zsh might be renamed to ohMyZsh as well in that case.

@fpletz fpletz merged commit 6a8aaf2 into master May 11, 2017
@fpletz fpletz deleted the rename-zsh-highlighting-option branch May 11, 2017 17:03
@Ma27
Copy link

Ma27 commented May 17, 2017

btw is it planned to merge this change back to upstream? Just asking because this is yet another diverge from upstream, but not that controversial IMHO.

@WilliButz
Copy link
Collaborator Author

I'd like to see it upstream as well but it's not up to me to make this decision :)

@fpletz
Copy link

fpletz commented May 20, 2017

Pushed upstream: NixOS@4e4f7a2

peti pushed a commit to NixOS/nixpkgs that referenced this pull request Jun 12, 2017
This is intended to provide better consistency with other NixOS modules.
Please refer to mayflower#21 for further information.

(cherry picked from commit a549596)
ambrop72 pushed a commit to ambrop72/nixpkgs that referenced this pull request Jun 21, 2017
This is intended to provide better consistency with other NixOS modules.
Please refer to mayflower#21 for further information.

(cherry picked from commit a549596)
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
This is intended to provide better consistency with other NixOS modules.
Please refer to mayflower#21 for further information.

(cherry picked from commit a549596)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants