-
Notifications
You must be signed in to change notification settings - Fork 228
Use only one global var for marking config folder tree #6610
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
Conversation
for more information, see https://pre-commit.ci
0db80ec
to
418cc2a
Compare
e3e89d1
to
d7f3684
Compare
02307b3
to
959a9ad
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6610 +/- ##
==========================================
+ Coverage 77.51% 77.90% +0.40%
==========================================
Files 560 567 +7
Lines 41444 42178 +734
==========================================
+ Hits 32120 32856 +736
+ Misses 9324 9322 -2 ☔ View full report in Codecov by Sentry. |
344fe89
to
1b26044
Compare
30bae8b
to
c4adbec
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If basedpyright
is happy with the code, there is probably nothing I can add :D
Looks good to me! Just a few minor comments.
Co-authored-by: Julian Geiger <[email protected]>
c9a7c04
to
95e305c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review and great suggestion @danielhollas @sphuber
I addressed all what you mentioned, and ready for another review I think.
Hi @danielhollas, please have a look at this when you get time. This is one of the fixes for a random failed test of #6620. Besides the naming of the get/set methods, it is ready from my side. I prefer to have the method name |
@unkcpz Thanks, I'll have a look today. One suggestion, it would help if the unrelated typing changes were extracted to a separate PR. But if it would be too much extra work don't worry about it.
I agree with the rename. |
d180e0b
to
aec9321
Compare
Not too many, and I agree it is better to not contaminate the major changes with fix type checking too much. I revert the changes in
Renamed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for sticking with this @unkcpz, very nice cleanup and the settings.py
file is now much easier to follow.
Thanks for the review and discussion! @sphuber @danielhollas @GeigerJ2 |
This bug was introduced in PR aiidateam#6610, where the temporary config folder path was changed unintentionally. Previously, it was set to `/tmp/pytest-.../config/.aiida`, but the new setup used `/tmp/pytest-.../config/`. As a result, the daemon_client fixture failed to detect the profile, leading to subtle issues. This bug was masked by two factors: - The CI setup (`setup.sh`) which preconfigures AiiDA profiles. - The fixture definitions in `tests/conftest.py`, which interfered with the intended isolation of aiida_instance. A follow-up commit will address this entanglement by clearly separating fixture configuration and test logic.
This bug was introduced in PR aiidateam#6610, where the temporary config folder path was changed unintentionally. Previously, it was set to `/tmp/pytest-.../config/.aiida`, but the new setup used `/tmp/pytest-.../config/`. As a result, the daemon_client fixture failed to detect the profile, leading to subtle issues. This bug was masked by two factors: - The CI setup (`setup.sh`) which preconfigures AiiDA profiles. - The fixture definitions in `tests/conftest.py`, which interfered with the intended isolation of aiida_instance. A follow-up commit will address this entanglement by clearly separating fixture configuration and test logic.
This bug was introduced in PR aiidateam#6610, where the temporary config folder path was changed unintentionally. Previously, it was set to `/tmp/pytest-.../config/.aiida`, but the new setup used `/tmp/pytest-.../config/`. As a result, the daemon_client fixture failed to detect the profile, leading to subtle issues. This bug was masked by two factors: - The CI setup (`setup.sh`) which preconfigures AiiDA profiles. - The fixture definitions in `tests/conftest.py`, which interfered with the intended isolation of aiida_instance. A follow-up commit will address this entanglement by clearly separating fixture configuration and test logic.
The problem is found when I worked on #6609. If the import of
DAEMON_DIR
moved to outside the function, the global var set in module scope and in test contest will not be reset when function is loading.The goal for a better design is to reduce the use of global variable. Now it is only the
glb_aiida_config_folder
(the originalAIIDA_CONFIG_FOLDER
). Other three are derived from this one. This lower the possibility of directly access the global variable without noticing its change.