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

Add instrument varian vistapro #729

Merged
merged 13 commits into from
Mar 22, 2018

Conversation

Lunga001
Copy link
Contributor

Description of the issue/feature this PR addresses

 Add instrument Varian Vista-PRO ICP

Linked issue: https://github.com/senaite/senaite.core/issues/

Current behavior before PR

  Instrument Varian Vista-PRO ICP not on the system

Desired behavior after PR is merged

 Add instrument Varian Vista-PRO ICP

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

Copy link
Member

@xispa xispa left a comment

Choose a reason for hiding this comment

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

Only very small (and cosmetic) changes @Lunga001 !, good work!

errors.append(_("No file selected"))

parser = VistaPROICPParser(infile)
if parser:
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this condition cause it will always be met.

elif sample == 'clientsid':
sam = ['getClientSampleID']
elif sample == 'sample_clientsid':
sam = ['getSampleID', 'getClientSampleID']
Copy link
Member

Choose a reason for hiding this comment

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

Maybe is out of the scope of this particular import interface, but I would be nice to move all the logic above (that is shared among almost all instruments) to somewhere else (e.g. a function get_instrument_import_search_criteria, etc. inside instruments.__init__.py)

Copy link
Contributor Author

@Lunga001 Lunga001 Mar 16, 2018

Choose a reason for hiding this comment

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

I've created a file called instruments/utils.py because I could not import get_instrument_import_search_criteria when it is on the instruments.__init__.py since icp.py is also imported on instruments.__init__.py

def parse_headerline(self, line):
""" Parses header lines
"""
if self._end_header is True:
Copy link
Member

Choose a reason for hiding this comment

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

Better use if self._end_header: instead. See PEP8 recommendations

return self.parse_headerline(line)
else:
return self.parse_resultsline(line)

Copy link
Member

Choose a reason for hiding this comment

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

I'd replace the code by the following:

if self._end_header:
    return self.parse_resultsline(line)
return self.parse_headerline(line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"total_results": self.getResultsTotalCount()}
)

def zeroValueDefaultInstrumentResults(self, column_name, result, line):
Copy link
Member

Choose a reason for hiding this comment

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

The name of this function is confusing. ' get_result(self, column_name, result, line)` would be better imo

result = float(result)
if result < 0.0:
result = 0.0
except ValueError:
Copy link
Member

Choose a reason for hiding this comment

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

You can take advantage of api.is_floatable and api.to_float here. Something like:

if api.is_floatable(result):
    result = api.to_float(result)
    return result > 0.0 and result or 0.0

self.err("No valid number ${result} in column (${column_name})",
...

return json.dumps(results)


class Export(BrowserView):
Copy link
Member

Choose a reason for hiding this comment

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

Cool!!!!

@Lunga001
Copy link
Contributor Author

Thanks for review @xispa I've pushed the requested changes

@xispa xispa merged commit 7c71549 into senaite:master Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants