-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Add new 7 instruments #617
Conversation
The title of the Pull Request is not self-explanatory and the description has links that redirect to a private issue tracker. If there is no issue in senaite.core's tracker related with this PR, please explain the functionality you are trying to accomplish in the description directly, with as much detail as possible. Also, avoid the use of IDs in titles and/or description that do not make sense inside I can see this PR contains multiple instrument interfaces. Please, much better if you open a PR for every single instrument interface instead of a PR with all them at once. It makes the whole thing very difficult to review. |
Thank you for the review @xispa and please forgive me for the long and non-descriptive PR. I will start with making the PRs for each instrument. |
Hi @Lunga001 if you update the title and description with as much detail as possible, I can review this PR all at once (but only this time). Not my intention to sound rude, but we are applying the same strict rules to all PRs, without exception, on behalf the quality of the code and thus, the quality of the product afterwards. Thanks! |
Thanks @xispa, much appreciated, will do right next time. |
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 @Lunga001 !
CHANGES.rst
Outdated
@@ -6,6 +6,7 @@ Changelog | |||
|
|||
**Added** | |||
|
|||
- Added 7 new Instruments |
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 line is not explanatory about the changes included in this Pull Request. Please, add a line for each instrument with the number of Pull Request, like:
- #617 Instrument import interface: Shimadzu GCMS-QP2010 SE
- #617 Instrument import interface: Shimadzu GCMS-TQ8030 GC/MS/MS
- #617 Instrument import interface: Shimadzu ICPE-9000 Multitype
...
This way is easy later to find out which instruments are supported and since when, just by doing a quick search for "Instrument import interface".
@@ -112,8 +112,7 @@ def getResultsTotalCount(self): | |||
""" | |||
count = 0 | |||
for val in self.getRawResults().values(): | |||
for row in val: | |||
count += len(val) | |||
count += len(val) |
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.
Good catch!
Description of the issue/feature this PR addresses
Adding the following 7 Instruments
Current behavior before PR
Desired behavior after PR is merged
Add 7 Instruments and write one doctest for all Instruments.
Please note that I've made a new folder called instruments under test/files/ and added the tests files there.
The reason I did not add a test file for an instrument like Abbott - m2000 Real Time is that Abbott - m2000 Real Time uses different form variables like "filename" instead of "instrument_results_file" and all these 7 instruments are using the default instrument pt template which is "exportimport/instruments/instrument.pt"
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.