- 
                Notifications
    You must be signed in to change notification settings 
- Fork 23
          test: use filename string instead of pathlib.Path to fix tests using diffpy.structure
          #128
        
          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
…`diffpy.structure`
| Warning! No news item is found for this PR. If this is a user-facing | 
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   67.05%   66.99%   -0.07%     
==========================================
  Files          25       25              
  Lines        3160     3163       +3     
==========================================
  Hits         2119     2119              
- Misses       1041     1044       +3     
 🚀 New features to boost your workflow:
 | 
| @sbillinge, it's ready for review. The "Check for News" failed because the branch was renamed. The correct news item was added in the PR. | 
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.
please see comments
        
          
                tests/test_pdf.py
              
                Outdated
          
        
      |  | ||
| stru = PDFFitStructure() | ||
| ciffile = datafile("ni.cif") | ||
| ciffile = str(ciffile) | 
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.
let's change the variable name to cif_path.
Please can you also make an issue on diffpy.structure to have loadStructure accept pathlib.Path or string, i.e., any valid path object.?
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.
Please can you also make an issue on diffpy.structure to have loadStructure accept pathlib.Path or string, i.e., any valid path object.?
Yes, there was already an issue diffpy/diffpy.structure#124
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.
@sbillinge, it's ready for review.
| stru = PDFFitStructure() | ||
| ciffile = datafile("ni.cif") | ||
| stru.read(ciffile) | ||
| cif_path = str(ciffile) | 
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.
renamed to cif_path
| pc.loadData(datafile("ni-q27r100-neutron.gr")) | ||
| ni = loadStructure(datafile("ni.cif")) | ||
| ciffile = datafile("ni.cif") | ||
| cif_path = str(ciffile) | 
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.
renamed to cif_path
What problem does this issue address?
Fix tests involving
diffpy.structureas mentioned in #90 (comment)What should the reviewer(s) do?
Please check the modifications.