app: config: Add support for appending to the config string #768
Conversation
carlescufi
force-pushed
the
config-append
branch
from
9301226 to
717a09c
Compare
carlescufi
force-pushed
the
config-append
branch
from
717a09c to
e46a0d4
Compare
|
I do have a concern about the appending if the value doesn't exist yet. Something like: $ west config -a manifest.group-filter ,+extrasCould result in an error because of the leading comma. |
I also thought about that, but did not want to overcomplicate the code. But yes, let me do that as well. |
carlescufi
force-pushed
the
config-append
branch
from
e46a0d4 to
7a7c2e3
Compare
carlescufi
force-pushed
the
config-append
branch
3 times, most recently
from
37e03bc to
69ac4f1
Compare
carlescufi
force-pushed
the
config-append
branch
3 times, most recently
from
c8d62f6 to
3cf5a27
Compare
carlescufi
force-pushed
the
config-append
branch
from
3cf5a27 to
4d8d948
Compare
yep, on it! |
carlescufi
force-pushed
the
config-append
branch
from
4d8d948 to
4260ed1
Compare
tests/test_config.py
Outdated
| @@ -229,6 +229,9 @@ def test_local_creation_with_topdir(): | |||
| assert 'pytest' not in cfg(f=GLOBAL) | |||
| assert cfg(f=LOCAL, topdir=str(topdir))['pytest']['key'] == 'val' | |||
|
|
|||
| def test_append_basic(): | |||
carlescufi
force-pushed
the
config-append
branch
from
4260ed1 to
081a3be
Compare
carlescufi
force-pushed
the
config-append
branch
from
cd91c73 to
edd489f
Compare
There was a problem hiding this comment.
@carlescufi please try to follow the formatting suggestions from ruff for NEW or CHANGED code but please avoid git churn in unchanged code, thanks!
Honestly I have to say I disagree with this approach, and I already told @pdgendt. I think we either reformat the codebase to match |
@pdgendt my guess is you just forgot to add a ruff-excludes.toml file, did you not? The way it's done today it's preventing folks to contribute until they fix the entire file they're touching, which sounds broken (and unintentional). FWIW this bit me here #713 too. |
Not really, this was the outcome after some offline discussions with @marc-hb, which were also reasonable, but not the same as the approach that was taken for Zephyr itself. |
So you mean you will be just ignoring the CI failures for the time being if they are for lines that were not changed by the contributor? Sorry if I am missing something obvious, but I don't understand why it was done differently than for Zephyr itself (plus, of course, I have no idea about the content and outcome of the offline discussions you're referring to ;)) |
Yeah, I understand the confusion, a GH job can only be green or red, nothing in between. The current idea was; contributors can be asked to update the format, but if only a few lines are touched, it can get merged without it. There aren't as many contributors to |
|
A large part of the discussion is online: #766 |
(1) means the git history is practically destroyed. Especially problematic considering the main author left.
Yes, formatting issues are obviously not blocking. As mentioned by Pieter, Github has unfortunately no yellow color. BTW this is not just a problem with formatting; every warning in every linter has the same CI problem.
Agree it could/should be more obvious. Pieter could you maybe rename "Format check" to "Format _suggestion"? Basically: try to express "yellow" in words.
We're even then because I don't know how it was done for Zephyr itself :-) In #766 I gave a real-world example where this approach worked well. It was not formatting but
An exclude file is interesting, is that how Zephyr solves this problem in general? Not just with formatting but with every linter? Unlike (1) and (2), this allows some form of incremental progress but there are still smaller "big bangs" where the entire git history gets broken on a file-by-file basis. An exclude file does not solve the problem of changes in the code or the configuration of the formatting tool itself. |
|
To be clear: I do not share Pieter's (and others') "enthusiasm" for formatting. Unlike C/C++, Python already enforces indentation = a little bit of formatting and in practice that seems enough to me, I mean I rarely ever scratch my head trying to understand badly formatted Python code. So, On the other hand, I use |
I would say the opposite, I "hate" formatting, as in, I don't want to spend time on it. Working on a project where code is in a certain style, I need to "learn" how formatting is done, and carefully curate whatever modifications or additions I do. For python projects I have worked with black in the past, and the ruff formatter is like black, but faster. They have a commitment towards compatibility too, so I have some trust in that the formatting doesn't change drastically.
And I agree, that's why I'm not a fan of blindly applying it to all files, but gradually. Either by exclusion or non-forcing checks. Both have downsides, but I'm happy to adjust. Currently tooling in and around Zephyr is the wild-wild-west. Maintainers get to call the shots, and for good reason, but this does have the downside of different approaches in each. I introduced ruff in west and Zephyr hoping to get a more uniform tooling codebase. And formatting in an uncompromising way. |
I don't know about Zephyr more broadly, but in As I said, C/C++ offers a lot more editing "creativity" than Python so maybe formatting has been a problem there but for What would be perfectly non-intrusive is some sort of
|
By "time spent", I meant what I do while developing, not commenting on contributions. I have developed in many different languages, mostly out of curiosity and learning new things. And languages like go, rust, zig, ... (yes all the new kids) come with formatting guidelines and tooling out of the box. Python has the PEP8 guideline which allows much more than what black or ruff currently output, but we currently don't comply either.
That was my earlier point on trying have uniform code across projects under the Zephyr umbrella, and without tooling to enforce it, it's always a never-ending discussion.
You can turn off formatting just as easy for code blocks, but this shouldn't be the default IMO. |
I like that too - but as you know that's not the reality for Python. Except for one very important thing: indentation.
I agree it becomes a never-ending discussion without tooling BUT only if maintainers decide to engage in that discussion in the first place. That has never been the case with |
I just remembered that |
Simplify the error handling of the different action flags (-l, -d and -D) by using a parser mutually exclusive group. Signed-off-by: Carles Cufi <[email protected]>
carlescufi
force-pushed
the
config-append
branch
from
edd489f to
924c306
Compare
carlescufi
force-pushed
the
config-append
branch
from
924c306 to
6407cac
Compare
|
|
||
| # Use a list instead of a string to get quoting right | ||
| cmd(['config', '--global', '-a', 'build.cmake-args', '--', | ||
| ' -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR']) |
There was a problem hiding this comment.
Nit, use outer " quotes so you don't need to escape the inner ' ones?
| ' -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR']) | |
| " -DEXTRA_CFLAGS='-Wextra -g0' -DFOO=BAR"]) |
There was a problem hiding this comment.
I had it like that before, but I personally actually prefer it this way, because it is consistent with the rest of the elements in the list.
There was a problem hiding this comment.
You could use " for inner quotes... unless it fails on Windows? As long as cmake does not run Windows should be fine...
I find backslashes a bit less readable but this is just test code so I don't actually care here.
There was a problem hiding this comment.
A few style suggestions + one question, otherwise LGTM!
once we are happy with that, we can discuss with the two of you which ruff complaints you'd like me to address.
As "usual", I think new and modified code should be "ruff-clean" while unchanged code should be... unchanged. Clicking on the link added by Pieter should help with that.
https://wiki.openstack.org/wiki/GitCommitMessages#Things_to_avoid_when_creating_commits
| self.die(f'option {args.name} not found in the {where.name.lower()} ' | ||
| 'configuration file') | ||
| args.value = value + args.value | ||
| self.write(args) |
There was a problem hiding this comment.
How come you don't need where here?
There was a problem hiding this comment.
The doc says:
If the configuration file to use is not set, reads use all three in
precedence order, and writes (including appends) use the local file.
we provide args to self.write(), so append behaves exactly like write as documented.
There was a problem hiding this comment.
I should have looked at the write() code sorry.
|
|
||
| # Use a list instead of a string to get quoting right | ||
| cmd(['config', '--global', '-a', 'build.cmake-args', '--', | ||
| ' -DEXTRA_CFLAGS=\'-Wextra -g0\' -DFOO=BAR']) |
There was a problem hiding this comment.
You could use " for inner quotes... unless it fails on Windows? As long as cmake does not run Windows should be fine...
I find backslashes a bit less readable but this is just test code so I don't actually care here.
| def test_append_novalue(): | ||
| with pytest.raises(subprocess.CalledProcessError) as exc_info: | ||
| cmd('config -a pytest.foo', stderr=subprocess.STDOUT) | ||
| # Get the output into a variable to simplify pytest error messages |
There was a problem hiding this comment.
I don't think this comment adds a lot of value. No big deal.
There was a problem hiding this comment.
I think it does, if you don't do this the error message thrown by pypi is unreadable.
There was a problem hiding this comment.
I think I misunderstood and thought this comment was referring to the code itself, not to its output.
| # Get the output into a variable to simplify pytest error messages | |
| # Use a variable to make the "assert" output readable |
Not important.
In some cases, and specifically in the manifest.group-filter and manifest.project-filter options, it is sometimes useful to be able to append to a value instead of replacing it completely. For example, assuming one wants to add to an existing group filter, without this patch the user needs to do: (assuming the group filter is currently +unstable,-optional, and the user wants to add +extras). > west config manifest.group-filter > west config manifest.group-filter +unstable,-optional,+extras With this patch instead: > west config -a manifest.group-filter ,+extras Signed-off-by: Carles Cufi <[email protected]>
carlescufi
force-pushed
the
config-append
branch
from
6407cac to
33d4230
Compare
| def test_append_novalue(): | ||
| with pytest.raises(subprocess.CalledProcessError) as exc_info: | ||
| cmd('config -a pytest.foo', stderr=subprocess.STDOUT) | ||
| # Get the output into a variable to simplify pytest error messages |
There was a problem hiding this comment.
I think I misunderstood and thought this comment was referring to the code itself, not to its output.
| # Get the output into a variable to simplify pytest error messages | |
| # Use a variable to make the "assert" output readable |
Not important.
| self.die(f'option {args.name} not found in the {where.name.lower()} ' | ||
| 'configuration file') | ||
| args.value = value + args.value | ||
| self.write(args) |
There was a problem hiding this comment.
I should have looked at the write() code sorry.
In some cases, and specifically in the manifest.group-filter and manifest.project-filter options, it is sometimes useful to be able to append to a value instead of replacing it completely.
For example, assuming one wants to add to an existing group filter, without this patch the user needs to do:
(assuming the group filter is currently +unstable,-optional, and the
user wants to add +extras).
$ west config manifest.group-filter
$ west config manifest.group-filter +unstable,-optional,+extras
With this patch instead:
$ west config -a manifest.group-filter ,+extras