Fix config profiles quickly get polluted with untouched settings#17158
Conversation
e83da50 to
b2a9089
Compare
|
@CyrilleB79 Would you be able to assist with testing to ensure we don't reintroduce anything from #14760 with this pr? |
b2a9089 to
e702421
Compare
|
@coderabbitai review |
WalkthroughThe changes in this pull request focus on enhancing the logic for handling configuration settings in the application. Modifications were made to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
user_docs/en/changes.md (1)
Line range hint
1-1: TODO: Add testsThis comment indicates that tests are currently missing for this module. Adding tests is important to ensure the code functions correctly and to prevent regressions when making changes.
Do you need any help with writing tests for this code? I'd be happy to provide guidance on effective test cases to cover. Let me know if you'd like me to open a GitHub issue to track the task of adding tests.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- source/config/init.py (3 hunks)
- user_docs/en/changes.md (1 hunks)
Additional context used
Path-based instructions (2)
source/config/__init__.py (2)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.user_docs/en/changes.md (3)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.
Pattern
user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)
Additional comments not posted (2)
source/config/__init__.py (2)
1148-1148: LGTM!The change to directly return
valwhencheckValidityisFalseis approved. This emphasizes that unvalidated values should not be cached, potentially improving the integrity of the cached data.
1293-1296: LGTM!The changes to the conditional checks for updating values are approved. They simplify the logic and enhance clarity regarding when updates should occur.
seanbudd
left a comment
There was a problem hiding this comment.
Is it possible to create a unit test which covers this case in tests/unit/test_config.py?
|
@seanbudd I just created unit tests and checked that the tests fail without the patch in this pr. |
Link to issue number:
Fixes #17157
Fixup of #14133
Partially reverts #15074
Summary of the issue:
When saving configuration profiles, unchanged values are yet saved to a profile.
Description of user facing changes
Configuration profiles should become less polluted after this change.
Description of development approach
Restored the behavior of #14133, mostly reverting #15074. There was namely a False assumption in #15074.
@seanbudd wrote:
In fact, the validation was not the problem here. Instead, when
__getitem__was called withcheckValidityset to False, the unvalidated value was yet saved in the cache. Therefore subsequent calls of__getitem__would always return the unvalidated cached value that would always be of the string type.Testing strategy:
Known issues with pull request:
None known
Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation