Remove parsing of command line arguments from CliSettingsSource.__init__.

[trygve-baerland]

As part of the discussions in #610 and #654 , PR #611 needs to be reverted due to breaking backwards compatability. In particular, it stems from CliSettingsSource.__init__ eagerly parsing command line arguments when passed with cli_parse_args.

This PR is meant as an alternative solution to the problem:

1. The call to _load_env_vars in CliSettingsSource.__init__ is removed.
2. Checks for CliSettingsSource.env_vars put in places where the caller needs the arguments to have been parsed. As far as I found, this turned out to be during serialization (in CliSettingsSource._serialized_args), and during CliSettingsSource.__call__.
3. I've also added a test that's almost identical to the example found in CLI integration with existing parser is broken. #654 .

NOTE: This PR is primarily meant as part of a discussion (and as a learning experience for me), so feel free to reject it on account of code quality or any other reason. Any feedback, though, is greatly appreciated.

[hramezani]

Thanks @trygve-baerland for the PR.

@kschwab can you take a look at this PR?

[kschwab]

Hey @trygve-baerland thanks for the PR. I agree with your intent. I think the case where this matters is during settings_customise_sources, where it should pull configurations from the settings_cls.

I would revert the above and original changes, as they are still breaking.

However, I think the best place to address this is here, where if we get a CLI source from settings_customise_sources, and it hasn't parsed the args, we parse them. I think the below should be all that is necessary:

custom_cli_source = [source for source in sources if isinstance(source, CliSettingsSource)]
if not custom_cli_source:
    # etc 
elif cli_parse_args and not custom_cli_source[0].env_vars:
    custom_cli_source[0](args=cli_parse_args)
# etc

[trygve-baerland]

Thank you for your feedback, @kschwab.

I think your approach makes a lot of sense. It also has the added benefit of being less prone to unintended side effects, which is what got us here in the first place :)

EDIT: I am totally fine with still doing the revert, but I don't see how these changes breaks backwards compatability. Not saying I'm at all certain it doesn't, just that I don't see it.

[trygve-baerland]

@kschwab : I've made the changes as you suggested (you were bang on with it).

Further, I've rebased onto #655, and can wait with this PR until that's been merged.
I've also added back the test that will be removed by #655 .

[hramezani]

Thanks @trygve-baerland for the update. I merged my PR.

[trygve-baerland]

Super, @hramezani ! I've rebased onto main, so I think it's good for a second look, @kschwab

[kschwab]

Looks good! Thanks @trygve-baerland for the changes.

[hramezani]

Thanks all 🙏