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

fix: updating of non-dict config values gives error #2364

Merged
merged 10 commits into from Aug 5, 2023

Conversation

padix-key
Copy link
Contributor

@padix-key padix-key commented Jul 19, 2023

Description

This PR addresses an issue in config merging: If a config value that is not a dict (e.g. it is None) is replaced by a dict an exception is raised, as update_config() tries to update the keys of the config, which is not possible (e.g. None has no keys). This PR fixes this problem by just overwriting the original value in this case (e.g. None is replaced by the new dictionary).

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

@padix-key
Copy link
Contributor Author

Now the updated tests are included

@padix-key padix-key changed the title Fix updating of non-dict config values fix: updating of non-dict config values gives error Jul 20, 2023
@johanneskoester
Copy link
Contributor

Seems like a testcase fails because the result is not as expected?

Necessary, as changing e.g. `None` to a new `dict` replaces the object instead of an in-place modification
@padix-key
Copy link
Contributor Author

Hi, The problem was the removal of the inner function: When replacing None with a new dict this is no in-place modification anymore. Hence, without a return value the config does not get updated in this case. I reenabled the inner function with return value to solve this issue and edited the PR description accordingly.

@johanneskoester johanneskoester enabled auto-merge (squash) August 4, 2023 13:29
auto-merge was automatically disabled August 4, 2023 13:47

Head branch was pushed to by a user without write access

@padix-key
Copy link
Contributor Author

Conceptually, the function was correct, only formatting changes of the module and the test output file was required in the failed jobs. My changes should hopefully fix this now.

@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johanneskoester johanneskoester merged commit b33aeec into snakemake:main Aug 5, 2023
7 checks passed
johanneskoester pushed a commit that referenced this pull request Aug 5, 2023
🤖 I have created a release *beep* *boop*
---


##
[7.32.1](v7.32.0...v7.32.1)
(2023-08-05)


### Bug Fixes

* add missing spaces between lines that get concatenated.
([#2268](#2268))
([7238458](7238458))
* better message about profile usage upon execution
([#2391](#2391))
([cf8aea5](cf8aea5))
* do not overwrite default resources setting in azure batch executor
([#2395](#2395))
([4aef3b9](4aef3b9))
* updating of non-dict config values gives error
([#2364](#2364))
([b33aeec](b33aeec))
* wrong rule names when nesting module imports
([#1817](#1817))
([65c79a4](65c79a4))


### Documentation

* basics.rst: suggest VS Code instead of deprecated Atom as IDE
([#2368](#2368))
([1357316](1357316))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants