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.

Sign up for GitHub

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

Jump to bottom

fix: get_writer errors if filename is none #4043

Merged
merged 8 commits into from
Mar 5, 2023
Merged

fix: get_writer errors if filename is none #4043

merged 8 commits into from
Mar 5, 2023

Conversation

jandom
Copy link
Contributor

@jandom jandom commented Feb 26, 2023 •
edited
Loading

Fixes a small bug

Changes made in this Pull Request:

  • catch a case when filename is truthy and format is uninitialized

PR Checklist

  • Tests? Added a simple unit test, maybe too much mocking, please review
  • Docs? Not applicable
  • CHANGELOG updated? Yes!
  • Issue raised/referenced? Nope

@jandom jandom self-assigned this Feb 26, 2023
@codecov
Copy link

codecov bot commented Feb 26, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (79e0c86) 93.53% compared to head (71b2365) 93.54%.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #4043   +/-   ##
========================================
  Coverage    93.53%   93.54%           
========================================
  Files          191      191           
  Lines        25063    25065    +2     
  Branches      4042     4042           
========================================
+ Hits         23443    23446    +3     
  Misses        1099     1099           
+ Partials       521      520    -1
Impacted Files Coverage Δ
package/MDAnalysis/selections/__init__.py 100.00% <100.00%> (+5.88%) ⬆️
package/MDAnalysis/analysis/psa.py 84.21% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jandom jandom force-pushed the jandom/fix/selections-get-writer-bug branch from 5b6e802 to f656944 Compare February 27, 2023 19:20
orbeckst
orbeckst reviewed Feb 27, 2023
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

What's the bug, i.e., do you have example code that triggers it?

package/CHANGELOG Outdated
@@ -19,6 +19,7 @@ The rules for this file:
* 2.5.0

Fixes
* Fix get_writer format uninitialized (PR #4043)
Copy link
Member

Choose a reason for hiding this comment

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

Also add your GH handle to the list of contributors.

Copy link
Member

Choose a reason for hiding this comment

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

still todo

Copy link
Member

Choose a reason for hiding this comment

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

I'm not going to make this a blocking request - but for the few folks that read the changelog to work out what changed in the package, could I ask for a little bit more information? Something like "Fix uninitialized format variable issue when calling selections.get_writer directly" would probably go a long way.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, especially when we don't have an issue to point to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, on it

@jandom jandom force-pushed the jandom/fix/selections-get-writer-bug branch from f656944 to 8c9b071 Compare February 27, 2023 20:29
@jandom
Copy link
Contributor Author

jandom commented Feb 27, 2023

It's a mini-bug that doesn't actually happen in the way the function is called.

However, if you call the function with just the format argument, it'll error

selections.get_writer(None, "blah")

This will error

@richardjgowers
Copy link
Member

7cpwc6

@jandom jandom requested a review from IAlibay February 28, 2023 09:03
@jandom jandom marked this pull request as ready for review February 28, 2023 09:03
@jandom jandom requested a review from orbeckst February 28, 2023 22:08
orbeckst
orbeckst requested changes Mar 1, 2023
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

lgtm, just add your GH handle to CHANGELOG

package/CHANGELOG Outdated
@@ -19,6 +19,7 @@ The rules for this file:
* 2.5.0

Fixes
* Fix get_writer format uninitialized (PR #4043)
Copy link
Member

Choose a reason for hiding this comment

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

still todo

@orbeckst orbeckst assigned orbeckst and unassigned jandom Mar 1, 2023
@jandom jandom requested a review from orbeckst March 3, 2023 16:40
orbeckst
orbeckst previously requested changes Mar 3, 2023
View reviewed changes
Copy link
Member

@orbeckst orbeckst 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 more explicit CHANGELOG entry, please, see https://github.com/MDAnalysis/mdanalysis/pull/4043/files#r1122512703

@orbeckst
Copy link
Member

orbeckst commented Mar 3, 2023

Not sure why CI is not running.

@jandom jandom requested a review from orbeckst March 4, 2023 11:24
@richardjgowers richardjgowers merged commit 55a5abd into MDAnalysis:develop Mar 5, 2023
@jandom jandom deleted the jandom/fix/selections-get-writer-bug branch March 5, 2023 16:21
@jandom
Copy link
Contributor Author

jandom commented Mar 5, 2023

Thanks for the merge, it's a small contribution but I hope to send more in

@IAlibay IAlibay added the defect label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardjgowers richardjgowers richardjgowers approved these changes

@IAlibay IAlibay Awaiting requested review from IAlibay

@orbeckst orbeckst Awaiting requested review from orbeckst

Assignees

@orbeckst orbeckst

Labels
defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jandom @richardjgowers @orbeckst @IAlibay