Skip to content
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 7 commits into from Dec 17, 2023
Merged

Conversation

RollingStar
Copy link
Contributor

@RollingStar RollingStar commented Nov 3, 2023

Description

Sorted the default config into categories, more or less. Added a FIXME and URL for an undocumented feature but otherwise it just re-arranges the existing config. No changes to substance.

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

Sorted the default config into categories, more or less. Added a FIXME and URL for an undocumented feature but otherwise it just re-arranges the existing config. No changes to substance.
Sort and categorize config_default.yaml
@JOJ0
Copy link
Member

JOJ0 commented Nov 16, 2023

Yeah why not sort this. It might help contributors to find a good place for their potential new default. I think it's an awesome idea personally since I often look into that file and still am overwhelmed with all the things that are in there :-)

Some things definitely are "opinionated" like maybe for some persons: "hardlinking" is the most natural thing to do with beets, haha :-), but I'm not all opposed to group them like that. For the "mainstream" beets users, probably your grouping is just right and you won't hurt much feelings with it ;-)


overwrite_null:
album: []
track: []
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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:

  1. sort config
  2. sort config page of docs
  3. split config page of docs into basic/advanced
  4. split config yaml into basic/advanced yamls (not sure how hard it would be to have cascading configs; I think JOJ0 does this now).
  5. move inline function definitions to their own user_inline.py file to take advantage of code markup and symbol definitions in IDEs

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.

Copy link
Member

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 of config_default.yaml and can just adjust exactly what they need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really hope that most people don't need to copy & paste the entirety of config_default.yaml

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.

Copy link
Member

@JOJ0 JOJ0 Nov 29, 2023

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:

  • Remove the comment about the undocumented feature but add a proper entry to the documentation. Or if that's not possible at this time or seems tedious, just leave an issue about a missing config entry.
  • Merge the sorted config_default.yaml / Merge this PR

PS: I want to find time to comment on other points and ideas you mentioned @RollingStar soon! :-)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Contributor Author

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.

Copy link
Member

@JOJ0 JOJ0 Dec 7, 2023

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.

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.

This PR is secretly part 1 of 3 or more:

  1. sort config

very good idea

  1. sort config page of docs

very good idea

  1. split config page of docs into basic/advanced

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!

  1. split config yaml into basic/advanced yamls (not sure how hard it would be to have cascading configs; I think JOJ0 does this now).

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

  1. move inline function definitions to their own user_inline.py file to take advantage of code markup and symbol definitions in IDEs

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 an inline_album.py and inline_item.py

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.

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?

@RollingStar
Copy link
Contributor Author

Some things definitely are "opinionated" like maybe for some persons: "hardlinking" is the most natural thing to do with beets, haha :-), but I'm not all opposed to group them like that. For the "mainstream" beets users, probably your grouping is just right and you won't hurt much feelings with it ;-)

Hardlinks makes a lot of sense. But they apparently don't work on Windows (can we confirm that?) so I think it's instantly not a novice-level feature.

https://beets.readthedocs.io/en/latest/reference/config.html#link

Lots of stuff on Google looks like you can hardlink or something like it on Windows. https://www.reddit.com/r/sonarr/comments/w8vt85/can_someone_plesae_explain_how_to_hardlink_the/

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

docs/changelog.rst Outdated Show resolved Hide resolved
RollingStar and others added 2 commits December 17, 2023 10:47
Co-authored-by: J0J0 Todos <2733783+JOJ0@users.noreply.github.com>
@RollingStar
Copy link
Contributor Author

Is this ready for merge now that I merged @JOJ0's suggestion?

@JOJ0 JOJ0 merged commit bcf180d into beetbox:master Dec 17, 2023
13 checks passed
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.

None yet

3 participants