-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sort and categorize config_default.yaml #4987
Merged
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9f5e6ed
Sort and categorize config_default.yaml
RollingStar ec3d9b9
Merge pull request #1 from RollingStar/config_sort
RollingStar 69493d0
Remove fixme
RollingStar ca1edab
Merge branch 'beetbox:master' into master
RollingStar ae8866f
Update changelog.rst for config_default
RollingStar 9f958b8
Update docs/changelog.rst
RollingStar d927262
Merge branch 'master' into master
RollingStar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't get docs to render on my PC so I can't test this. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very interesting catch! Do you want to quickly open a "missing docs" issue for it. Someone "who knows" might want do document it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW it's not too uncommon for us to have undocumented config options. The way I think about this is that an undocumented option is better than not having the option at all (i.e., to have a magic number in the code). Sometimes these are undocumented intentionally, because we mostly hope people will not use them except in special circumstances, and sometimes we just haven't gotten around to it yet. In yet a third category, it might just be so specific that it seems like not worth the space in the docs page!
I'm not 100% certain which category to put
overwrite_null
in. Probably just missing docs that we should add? But maybe it's just hyper-specific with an audience that rounds down to zero. FWIW, it was added in #3133.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case of this is I'm in my config or config_default more often than you might think. But it feels like I'm changing "basic" parts more often. Plugins, timid/quiet, replace/inline/paths are probably my most common changes. The esoteric and undocumented parts most people will never touch. This PR is about taming the beast of config_default and make it a more useful starting point for a user config.
This PR is secretly part 1 of 3 or more:
In an ideal world I think every config option would at least point to the PR it was added, or the part of the code it exists in. That's often what I'm looking at when trying to figure out if a config feature is something I need to change. But even linking that stuff may be too much work for very esoteric options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on the docs page to prioritize the most important stuff seems great!
While it also seems like a good idea to organize
config_default.yaml
, I am, in general, not sure we should be relying on it to be a "starting point" for user configs… I really hope that most people don't need to copy & paste the entirety ofconfig_default.yaml
and can just adjust exactly what they need to change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm not married to that idea. The reason I started with copying config_default for my personal config is I wanted to read through the config page and make sure I wasn't "missing" any options that would be helpful. Keeping the default value doesn't hurt anything (unless the value changes). [Also, I think part of it was I'm using the LSIO container and they change the config to their own default, and I wanted to change it back and take back control of the config.]
I wonder if there's a way to give people a blank config with the categories in this PR already filled in (possibly in the docs page). The user is free to use that or not, but it might help them keep some semblance of organization. Configs can get out of hand.
A sorted config_default also helps with maintenance on our end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest as a first step we could finalize this PR by doing these things:
PS: I want to find time to comment on other points and ideas you mentioned @RollingStar soon! :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the undocumented feature to the docs when i sort the page to match the new default yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel you. Often I find myself changing things back and forth between beets runs. What I find most helpful is controlling things via cli options. If we don't have them we should add them. Recently @mgoltzsche opened a bunch of PR's doing just that, which I very much appreciate.
very good idea
very good idea
sometimes hard to decide what's basic and what not. that's what I actually tried to say with my (probably bad) hardlinking example. I think it's rather advanced. Others might think differently.
All that aside please go ahead with that idea , we can nitpick about it over there :-) Great idea!
Not really like that but I use config overlays as a last resort solution. I also use shell aliases heavily. Best in my opinion is having complex beet commands consisting of --long-options (for readability) but saving them in .zshrc. If you forgot what it was
alias somebeetalias
prints it.also
-c overlay.yaml
could be contained in such a shell alias. For example sometimes I want Spotify as the preferred source. Stuff like that (source score) is not layed out as--options
About all that I wanted to make a discussions/showandtell post for a long time now already and never find the time.
For some things simply including via
include:
is enough...I do that with longish things like smartplaylist defs.one more thing helpful, especially when coding and reviewing beets stuff: custom shell prompts depending on python venv, $BEETSDIR, ... all controllable/switchable via zsh aliases
That is an awesome idea and might not even be extremly hard to implement. It should be configurable though so existing inline configs dont break. Maybe a new config option that enables that
inline:
is ignored but a separate inline.py is looked for in $BEETSDIR. Or even better, look for aninline_album.py
andinline_item.py
Indeed often I look up where and why something was implemented. But keeping track of that is impossible. The best thing for that would be our changelog. Usually it links to an issue though, so after looking that up the PR that closed it needs to be looked up after that in the often messy github message history.
Should we keep track not only about the issue but also the PR implementing it, right within the changelog? Would that improve something?