Remove class constants from element factory constructors #9
Conversation
…al strip function.
s4/clarity/artifact.py
Outdated
| @@ -1,6 +1,6 @@ | |||
| # Copyright 2016 Semaphore Solutions, Inc. | |||
| # --------------------------------------------------------------------------- | |||
|
|
|||
| from s4.clarity._internal.factory import BatchFlags | |||
There was a problem hiding this comment.
I would move the definition of BatchFlags to ClarityElement to avoid having to import in every ClarityElement.
There was a problem hiding this comment.
We still have to import it, just from .element instead of from .factory, which sucks, but it is a better place for the definition. I also explictly coded the default of none in.
s4/clarity/_internal/factory.py
Outdated
| if hasattr(self.element_class, 'NAME_ATTRIBUTE'): | ||
| return self.element_class.NAME_ATTRIBUTE | ||
| return "name" |
There was a problem hiding this comment.
This whole thing could just be return getattr(self.element_class, 'NAME_ATTRIBUTE', "name"). (I also don't know why it's a function, since why would anyone subclass and override it, but I guess if you really want it to be that's fine)
There was a problem hiding this comment.
They were separated out to remove the logic of determining where the name attribute was to be found from the logic of the constructor.
I really like the change you have of passing the default to getattr, that is small enough to put inline into the init.
| # Using the same RegEx query select out the hostname from the URI | ||
| return hostname_match.group(1) | ||
|
|
||
| def _get_environment(self): |
There was a problem hiding this comment.
Now that this is a function and nice and clean, it seems like this should be overrideable by passing a param into the constructor, so I can tell the LIMS object it is a test LIMS explicitly.
There was a problem hiding this comment.
Yeah.... I mean the whole thing is pretty suspect in my opinion. I don't think that we can actually declare the machine objective based on the name. Like, if we get a client called GeneTest and they get a server called clarity.genetest.com we will think that every machine, other than their dev machine, is the test machine.
Do we ever consume the output of this variable? It is unrelated to communicating with Clarity.
I would vote to remove it.
There was a problem hiding this comment.
You're probably right. Personally I wouldn't bother trying to remove it as part of this PR since this has been in limbo for a while and should get merged, might as well do it separately so we can argue about it there instead. (or I can after this is merged)
s4/clarity/_internal/factory.py
Outdated
| @@ -38,12 +38,13 @@ def __init__(self, lims, element_class): | |||
| """ | |||
| Creates a new factory to provide interface between Clarity LIMS and the client software. | |||
There was a problem hiding this comment.
@AvroNelson I meant this line was the one with the typo, sorry :D
|
I made the doc change (and then did some typing changes) in my own PR to Avro's branch. @AvroNelson : AvroNelson#1 for you to review (it will merge onto this branch) |
Moving the Batch Flags, Request Path and Name Attribute values out of the Element Factory initializers and into class constants on the actual element objects. Added some expanded comments and unit tests to verify expected behaviour.
Multiple
can_batch_X()methods on the ElementFactory have the values cast to bool as that is what they report to return, but were returning int values.