fix: Fix the hierarchy associated with ProfileParser.parse_file() and PDFParser.parse_file() - #192
fix: Fix the hierarchy associated with ProfileParser.parse_file() and PDFParser.parse_file()#192cadenmyers13 wants to merge 10 commits into
ProfileParser.parse_file() and PDFParser.parse_file()#192Conversation
|
@sbillinge ready for review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.3.0 #192 +/- ##
==========================================
+ Coverage 79.08% 81.44% +2.35%
==========================================
Files 25 25
Lines 3821 3837 +16
==========================================
+ Hits 3022 3125 +103
+ Misses 799 712 -87
🚀 New features to boost your workflow:
|
|
|
||
| self.select_bank(0) | ||
| return | ||
| If a subclass overrides `parse_string`, e.g. |
There was a problem hiding this comment.
when you say overrides do you mean overloads? Or is overrides a recognized term of art? I am not sure is why I am asking (and I guess Claude may have been involved and Claude generally uses the right terminology). I suggest to check this and then correct it everywhere if it needs it, or not if not.
There was a problem hiding this comment.
@sbillinge Overriding is when a subclass redefines a method it inherits, so override is correct in this case. "Override" is not used anywhere else so we should be good on updating
| The path to the data file. | ||
| """ | ||
| parser = ProfileParser() | ||
| parser = PDFParser() |
There was a problem hiding this comment.
please check this change is correct. Didn't we move from PDFParser to ProfileParser because the parser is more general than PDF? For example, couldn't it parse I(Q) data or anything else?
There was a problem hiding this comment.
@sbillinge We did and this changes it back. I changed it back because it seems like the intended design of ProfileParser was to be an inherited class for any parser in the future. For example, PDFParser inherits ProfileParser which contains the templating for a parser object. So this could be extended to other parsers, say, an IQParser. Each parser can be designed for a specific file format. In this case, PDFParser is for pdfgetx3 .gr files. It still uses load_data from diffpy.utils though which is and improvement from what it was before
There was a problem hiding this comment.
right, but getx3 files include iq files and sq and fq files which don't have PDF data in but could be read with the same parser. Also, I think .chi files coming from pyfai so it is not even a getx3 standard. How about maybe calling it GetxParser or something like that?
| # reads generic column data instead. These tests pin that boundary. | ||
|
|
||
|
|
||
| def test_parse_file_extracts_pdf_metadata(datafile): |
There was a problem hiding this comment.
Is there a reason these tests are not using pytest.mark.parametrize and the group standards for annotating the different cases?
There was a problem hiding this comment.
@sbillinge This is testing that the metadata is actually extracted from an example data file. I didn't think more cases were needed. In tests with only one case do we still want to use parameterize?
There was a problem hiding this comment.
I did parameterize the deprecated function though
There was a problem hiding this comment.
I saw that, but was seeing a whole series of tests, each testing one thing, so I thought it could be paramaterized.
|
|
||
|
|
||
| def test_parse_file_bad_parser( | ||
| parser_datafiles, |
There was a problem hiding this comment.
again here, we seem to have lost our paramatrize pattern. It makes it harder to follow
There was a problem hiding this comment.
@sbillinge touche, will make those changes
|
@sbillinge ready for review |
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
That looks better, thanks. Please see my comment about the naming though.
2 participants
A few bugs were caught here by claude.
PDFContribution.loadData()was discarding PDF-specific metadata because it didn't usePDFParserto parse data. This class isnt used much in practice but good to fix.ProfileParser.parse_file()parses metadata file like a string rather than by line. Now it checks whether a subclass (likePDFParser) overrides this behavior before parsing. This was behavior I changed earlier which I reverted back to because it makesProfileParsermore extensible.pushFitHookstopush_fit_hooks, so that is changed.Also some docs changes. All of this is backwards compatible with the deprecated functions. More tests have been added. ProfileParser did not have many tests to begin with.