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 typo in checking for ignore_nonexisting in LocalTransport #6471

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

MichaelGoulding
Copy link
Contributor

No description provided.

@mbercx
Copy link
Member

mbercx commented Jun 12, 2024

Thanks for the contribution @MichaelGoulding! I can see how the fact that the method looks for ignore_noexisting in the kwargs is problematic... Especially since other methods use an ignore_existing input (thankfully not popping kwargs). I'm a bit apprehensive towards fixing this directly, however, since users may be relying on the (unintuitive/incorrect) ignore_noexisting input and hence this would technically be a backwards-incompatible change.

I'll work on a deprecation path instead, and also add some more tests to make sure we don't break existing behaviour.

@MichaelGoulding
Copy link
Contributor Author

@mbercx The issue is that I when I switched an existing workflow that was using SshTransport to instead use LocalTransport, the workflow no longer worked. Perhaps LocalTransport could support both names for now?

@sphuber
Copy link
Contributor

sphuber commented Jul 5, 2024

@mbercx I have added the backwards compatibility fix. I want to release a patch version soonish. Could you have a quick look? Thanks!

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.82%. Comparing base (ef60b66) to head (465251c).
Report is 116 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/transports/plugins/local.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6471      +/-   ##
==========================================
+ Coverage   77.51%   77.82%   +0.32%     
==========================================
  Files         560      564       +4     
  Lines       41444    41922     +478     
==========================================
+ Hits        32120    32621     +501     
+ Misses       9324     9301      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion:

I noticed we also have ignore_existing somewhere.
Since you are raising an deprecation warning to version 3.
How about we deprecate all the three (ignore_existing, ignore_noexisting, ignore_nonexisting) in favor of exist_ok ?
Also they have a reversed logic, every time I have to translate in my mind -1 * -1 = +1 😄

@sphuber
Copy link
Contributor

sphuber commented Jul 16, 2024

Just a suggestion:

I noticed we also have ignore_existing somewhere. Since you are raising an deprecation warning to version 3. How about we deprecate all the three (ignore_existing, ignore_noexisting, ignore_nonexisting) in favor of exist_ok ? Also they have a reversed logic, every time I have to translate in my mind -1 * -1 = +1 😄

I think it is a good suggestion, but that is used in many places of the interface and would be a considerable change. I think that should be left to a separate PR if we decide to implement that name change. Feel free to open an issue to track it

Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @sphuber! Sorry for the slow review

The `LocalTransport.put` method contained a typo and so would check for
the `ignore_noexisting` argument instead of `ignore_nonexisting`. The
typo is corrected but for backwards compatibility the method continues
to check for the misspelled version emitting a deprecation warning.
@sphuber sphuber merged commit de83e2c into aiidateam:main Jul 17, 2024
10 of 11 checks passed
sphuber pushed a commit that referenced this pull request Aug 7, 2024
The `LocalTransport.put` method contained a typo and so would check for
the `ignore_noexisting` argument instead of `ignore_nonexisting`. The
typo is corrected but for backwards compatibility the method continues
to check for the misspelled version emitting a deprecation warning.

Cherry-pick: de83e2c
mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
…am#6471)

The `LocalTransport.put` method contained a typo and so would check for
the `ignore_noexisting` argument instead of `ignore_nonexisting`. The
typo is corrected but for backwards compatibility the method continues
to check for the misspelled version emitting a deprecation warning.
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

4 participants