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

Make it easier to override config file basename #418

Closed
bitprophet opened this issue Jan 6, 2017 · 7 comments
Closed

Make it easier to override config file basename #418

bitprophet opened this issue Jan 6, 2017 · 7 comments

Comments

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jan 6, 2017

This is technically a spinoff of #276; I don't see code about it in #278.

tl;dr right now the only way to change the <whatever>.yml (etc etc) sought by the config stuff is to manually subclass and modify Config, and then to modify a handful of spots annoyingly.

Config.__init__ allows overriding the various prefixes, but only individually, and it's moot because one cannot currently override those kwargs in Program - only the config_class!

This needs to be a lot easier:

  • Just change one spot and have that affect all the prefixes, so /etc/foobar.yml and ~/.foobar.yml and etc.
  • I don't see why we can't just use Program.name or whatever for this, either
  • May also want to add something like config_kwargs to Program in case users need to modify other kwargs and otherwise do not need to subclass; forced subclassing is inelegant and certainly was not my intention at the time.
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jan 16, 2017

One semi-minor wrinkle: we can't rely on Program because it can't affect the pure-library use case, which is a thing for client code like Fabric 2. So the core value driving these filenames needs to be internal to Config (as the prefixes currently are).

Actually kinda moot, because when I look, I realize Program.name (and Program.binary) are almost-but-not-quite what we want anyway: name is a proper name used for version/etc display, not lowercased, and binary is also for display purposes (eg inv[oke], fab[ric] etc).

So what'll happen is a "full" rebranding of Invoke needs:

  • Program instantiation (with or without subclassing), with or without name/binary values - they'll default to permutations on the installed binstub name specified in setup.py, which becomes the "actual" place one normally configures the program name.
    • So e.g. in setup.py, one does entry_points = {'console_scripts': 'myprogram = myprogram.main:program.run'} (with that first myprogram being the actual name)
  • Config subclassing, with (typically) an update to .global_defaults overriding whatever the new config file prefix option is (config_name or something) to the desired value (whether that's simply myprogram or something else)
  • And of course, handing that new Config class (or an instance, or kwargs, as per the other bullet points in description) to Program so it defaults to the subclass instead of Invoke's default.

Then users would do one of:

  • call the new package's binstub, e.g. myprogram, which would display as Myprogram 1.1.0, myprogram [--help] [--opts], etc, if using from CLI
  • import the new package's local Config subclass (from myprogram import Config) or permutations on Context that refer to the former, if using via code/REPL

And both situations would result in seeking e.g. ./myprogram.json for config file paths.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Feb 1, 2017

Wrote out a bunch of thoughts on config_kwargs (tl;dr yes config_kwargs is necessary instead of eg taking a Config instance directly - the Config really needs to be instantiated after CLI parsing has occurred!) then realized the use case I described - needing to stuff more runtime params into a Config subclass - doesn't actually require that, but instead requires tweaking more innards of Program to make it easier for subclasses to get in between the parse step and the instantiation step.

We do still want config_kwargs too, though, for folks who don't otherwise want a subclass.

Kinda mashing two things together here but oh well.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Apr 13, 2017

Looking at this for real now, and realizing that Program and setup.py really have nothing to do with config file loading, because as touched on above, this all needs to work in the absence of any CLI hooks. So if anything, Program should take some cues from the Config class given to it, and not the other way around.

However that (moving name/binary into Config) is not strictly required for my needs at the moment (getting a client lib like Fabric 2 able to load fabric.yaml instead of invoke.yaml) so I'm punting on it, even tho it means a "full rebrand" involves touching 2 different places. (There may be no good solution there anyways, since the defaults for name/binary come from setup.py or rather the resulting argv[0]; all we could get out of the Config class are overrides to that value.)

Believe all I need for right now is to update Config to use the aforementioned config_name or whatnot, and make it easy for a subclassed Config to override that (should already be possible).

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Apr 13, 2017

Question is whether this should be another kwarg in Config.__init__ or if it ought to come from inside the config data (the defaults) itself.

The former is more obvious and straightforward, but requires a config_kwargs in Program as discussed above. It's also only really useful for the default/root config used in the CLI use case, so it fails from the library/pure-API perspective - one would have to give it anytime one made a Config anywhere else. No good.

The latter feels kind of strange but allows a subclass to 'hardcode' the value within itself, and is something I've had to do before already (Fabric 2's subclass uses it to determine whether to load SSH conf files in its __init__).

Actually, there's the third option, a class-level attribute, which feels cleanest anyways even if it's not a pattern I have followed strongly in these codebases (but it's common in e.g. Django and other ORMs for just one example). It's still "defined by the class" but isn't floating around inside the config dicts, which would otherwise imply it can be modified at various config levels (which...it can't...as it determines how many of those other dicts are even loaded!)

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Apr 13, 2017

Another angle, the existing Config.init kwarg env_prefix is exactly in this space and wants very similar treatment; right now I've been calling it via a subclassed Program that changes its config_kwargs method (don't remember if I set that up before or after writing this ticket originally...hrm) but as I just noted this stuff wants to live more in Config itself.

Will probably move it to be same as whatever I do for config_name, again probably just class attributes and not init kwargs. Init kwargs are handy for writing unit tests but they feel odd otherwise (again given a world where Configs are often created w/o any init kwargs) so I shouldn't let the APIs be quite that test-driven...heh.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Apr 13, 2017

Wrote up some DDD that boils down to "use prefix (or the more specific file_prefix / env_prefix) class attributes for this". In doing so I realize we probably want to move nearly all non-data-bearing Config kwargs to this approach, but I'm too lazy to go through and change all of that + all of its tests right now :( but noting for the record.

bitprophet added a commit that referenced this issue Apr 13, 2017
bitprophet added a commit that referenced this issue Apr 13, 2017
bitprophet added a commit that referenced this issue Apr 13, 2017
bitprophet added a commit that referenced this issue Apr 13, 2017
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Apr 15, 2017

Neglected to note in a commit msg that this is done, but...it's done and Fabric 2 is now utilizing the new class attrs for Config so that it loads eg fabric.yaml and respects FABRIC_RUN_HIDE.

(Whether it should potentially respect both prefixes/filename types, is another matter that can be tackled later...)

@bitprophet bitprophet closed this Apr 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.