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

v0.1.6: Overriding sub-setting category nulls other inherited settings (#4348) #9872

Open
LTourinho opened this issue Feb 7, 2024 · 17 comments
Labels
configuration Related to settings and configuration

Comments

@LTourinho
Copy link

LTourinho commented Feb 7, 2024

Very similar to the issue described in #4348. I have a root dir with a file that looks a bit like:

proj/.ruff.toml:

[lint.isort]
section-order = ["standard-library", "common_pkgs", "third-party", "same_subcomp"]
[lint.isort.sections]
"common_pkgs" = ["numpy", "behave", ]
"same_subcomp"= []

proj/subdir/.ruff.toml:

extend = "../.ruff.toml"
[lint.isort.sections]
"same_subcomp" = ["<pattern>"]

Using a command such as ruff proj/subdir/<some_file> returns me an error like:

warning: section-order contains unknown section: UserDefined("common_pkgs")

Which means that isort.sections is being completely overwritten, when I expected to only overwrite the value of "same_subcomp". I can also confirm that the sorting of imports is not behaving as expected, as it will put numpy in the middle of the third party, when I want it to have its own block.

If this is the intended behavior, which I assumed it wasn't given issue #4348, is there a simple way I can achieve my intended behavior without copying the definition of common_pkgs to the .ruff.toml in every subdir?

Thanks in advance.

@zanieb zanieb changed the title At version 0.1.6, I am experiencing the same bug as shown in https://github.com/astral-sh/ruff/issues/4348 v0.1.6: Overriding sub-setting category nulls other inherited settings (#4348) Feb 7, 2024
@zanieb zanieb added the bug Something isn't working label Feb 7, 2024
@charliermarsh
Copy link
Member

Yeah, I think this should probably be considered a bug.

@LTourinho
Copy link
Author

Hi, I see that the PR is approved and checks are green. What would be the ETA on this bugfix delivery?

@charliermarsh
Copy link
Member

@LTourinho -- It's technically a breaking change, so it will go out in v0.3.

@charliermarsh
Copy link
Member

@LTourinho -- Sorry for the delay, we decided to defer this to v0.4 because we need to figure out a way to allow extended configs to unset these.

@LTourinho
Copy link
Author

@LTourinho -- Sorry for the delay, we decided to defer this to v0.4 because we need to figure out a way to allow extended configs to unset these.

No problem. Is the suggestion of setting to an "empty" object too bad in that regard? (The comment I made on the PR)

@LTourinho
Copy link
Author

Hi @charliermarsh, as the PR was closed, what are the plans on fixing this issue?

@MichaReiser
Copy link
Member

Hi @charliermarsh, as the PR was closed, what are the plans on fixing this issue?

Sorry that was not intentional. I cleaned up my stale branches and thought it's safe to delete any branch starting with charliemarsh. I restored the branch and reopened the PR.

@charliermarsh
Copy link
Member

Thanks Micha, I meant to ask about that too :)

@LTourinho
Copy link
Author

Hi guys, is there a reason this was postponed to 0.5? No agreement on how to unset values?

@MichaReiser
Copy link
Member

MichaReiser commented Apr 29, 2024

Hi guys, is there a reason this was postponed to 0.5? No agreement on how to unset values?

There was no specific reason other than time constraints. We didn't want to block the 0.4 release by making this decision.

@charliermarsh
Copy link
Member

0.4 was focused on the parser, we punted other breaking changes to 0.5 which we expect to release in the next few weeks.

@MichaReiser

This comment was marked as outdated.

@LTourinho
Copy link
Author

Hi, I see that the previous solution pull request was closed. Is there no alternatives? Is this being picked up in any way?

@MichaReiser
Copy link
Member

Unfortunately, I know of no alternative. We aren't actively working on this right now but are open to ideas. As little as I like that, a short-term solution could be an extend option. This comment explains why the other PR was closed)

@LTourinho
Copy link
Author

LTourinho commented Jul 4, 2024

Could there be a different keyword? E.g.: merge-into = "../.base_ruff.toml"

Then the breaking functionality would only happen to those who explicitly use the new keyword.

@MichaReiser
Copy link
Member

MichaReiser commented Jul 5, 2024

Could there be a different keyword? E.g.: merge-into = "../.base_ruff.toml"

I would prefer not to but I won't rule it out. Either way, deciding on adding a different extension strategy isn't something we want to do lightly because it adds a lot of complexity and also makes it harder for users to understand what's happening.

@KotlinIsland
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants