Add support for command-specific configuration sections

[chrysle]

Addresses #1933 (comment)

This introduces configuration sections specific to pip-compile and pip-sync.

I'd like to refactor the make_config_file() pytest fixture in a follow-up PR, to introduce tests for this feature using multiple configuration sections.

Contributor checklist

• Included tests for the changes.
• PR title is short, clear, and ready to be included in the user-facing changelog.

Maintainer checklist

• Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
• Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

[webknjaz]

Could you adjust the PR title to match the description? Currently it reads as if you're adding features but the text suggests that it's tests.
So it wouldn't hurt to be more specific — it'll end up in the changelog, after all.

[chrysle]

Thanks, it's actually this pull request I'd like to add more tests for (codecov is failing), but that needs some refactoring and doesn't really fit in. I've adjusted the description.

[webknjaz]

Ah, though I still don't understand what's tool-specific configuration. That title is just to generic to mean anything to me. It's quite unspecific wrt what it refers to. We already have pip-tools specific config, after all. That's the tool, right?

[chrysle]

We already have pip-tools specific config, after all. That's the tool, right?

Yeah, but that is the global configuration (for both pip-compile and pip-sync). I added the possibility to specify configuration values only for those commands. I added some docs to the README.

Is the title better now?

[webknjaz]

Is the title better now?

Much better, thanks! Titles shows up in email notifications among other places and tend to be a factor in deciding whether to even look inside. This is especially important when inbox is overflowing and people need to prioritize + figure out which notifications to discard. An accurate title helps draw attention of people who might be affected by the patch.

[webknjaz]

Initial review. I foresee the need to refactor things here and avoid complicating things.

[webknjaz]

I don't think it's a good idea to return a tuple. If it is really necessary to have all these bits of data, wrap them into an object (dataclass?) and return that.

Though, I'd argue it should return a single config instead. Maybe, we need to pass an argument telling it for which command that config is to be extracted..

[chrysle]

Yes, I can move the test below to parse_config_file().

[theryanwalker]

Suggested change

	piptools_config, pipcompile_config, pipsync_config = parse_config_file(
	config = Config.parse_file(config_file)

Given that you're in the process of refactoring this, how about introducing a Config class in piptools/config.py and moving the related logic there? This is an example of what the Config might look like:

@dataclass
class Config:
    pip_tools: dict[str, Any]
    pip_compile: dict[str, Any]
    pip_sync: dict[str, Any]
    
    @classmethod
    def parse_file(filename):
        ...

    def validate(self):
        ...
        
    def normalize_keys(self):
        ...
    
    def invert_negative_bool_options(self, ctx):
        ...
        
    def get_default_map() -> dict[str, Any]:
        ...
     
    def assign_to_cli(ctx):
        ...    

[chrysle]

@atugushev What do you think, should we dedicate an own file to that?

[atugushev]

Sounds great 👍🏻

[webknjaz]

@theryanwalker I like it, though I'd probably go for a different construction/classmethod name. How about Config.from_file_path()?

[webknjaz]

@chrysle looks like this also needs to be rebased due to conflicts in tests.

[chrysle]

Thank you for your suggestions and sorry for the long delay.
My code style is simply terrible... My new year's resolution definitely includes paying more attention in computer science class.

[webknjaz]

@chrysle hey, no problem! This is open source and nobody expects free labor to be a top priority of every contributor.

Also, the code style is something that evolves and is subjective most of the time. People are only expected to unconditionally adhere what automatic checks say. Otherwise, code reviews is a multi-party discussion that exists to agree on things and receive insights from other people that aren't related to code style.

At times, it may feel like some comments are about code style at glance, but usually, there's more important underlying issues that motivate people to make suggestions. It's mostly a perception thing. I know that my comments are often seen as style nitpicks while I write them with maintainability in mind, informed by architectural considerations, usually.

Here's a few links that I collected about code reviews, by the way: https://gist.github.com/webknjaz/2aa9c7a43b51c1385260ff87e0cbb9e3.

P.S. The linkcheck job is failing because jazzband.co has a TLS certificate that expired on Dec 30 last year and wasn't auto-renewed for some reason. A tagged @jezdez on matrix so he could take a look.
This blocks merging the PR, but once fixed, restarting that one failing job should unblock us.

[chrysle]

Thank you for your encouraging words and the time you invest into reviewing my code!
I'll also still have to solve the mystery how people get the right ideas for founding successful open source projects ;-)