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

enable 384 reservoir stamping if indicated in container_type.capabilities #300

Merged
merged 18 commits into from May 25, 2021

Conversation

aaronkarlsberg1
Copy link
Collaborator

@aaronkarlsberg1 aaronkarlsberg1 commented Apr 27, 2021

Instead of adding is_reservoir attribute, use the container_type.capabilities list to verify if plate is compatible with SBS384 transfers despite not having 384 wells.

  • :release:7.9.0 <2021-05-18>
  • :feature:301 Allow reservoir stamping for plates with shape SBS384 provided that container has capability: sbs384_compatible. Capability added to container RESSW384LP.
  • :support:294 Add tests in util to check if container type is a compatible reservoir

Please Land this PR after the stdv-665 port liquid_handle_dispense pr to respect versioning.

link to task

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2021

Codecov Report

Merging #300 (4322ae2) into master (948fca5) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #300   +/-   ##
=======================================
  Coverage   82.11%   82.11%           
=======================================
  Files          20       20           
  Lines        3803     3803           
=======================================
  Hits         3123     3123           
  Misses        680      680           
Impacted Files Coverage Δ
autoprotocol/container_type.py 99.00% <ø> (ø)
autoprotocol/version.py 0.00% <0.00%> (ø)
autoprotocol/util.py 83.87% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 948fca5...4322ae2. Read the comment docs.

@aaronkarlsberg1
Copy link
Collaborator Author

See former (now closed) pull request to compare different implementations:
[https://github.com//pull/299]

if (
shape["format"] == "SBS384"
and container_wells < 384
and "sbs384_compatible" not in container_type.capabilities
Copy link
Contributor

Choose a reason for hiding this comment

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

container type capabilities are usually the names of the operations/instructions that can be performed on a container type. sbs384_compatible will never be found in the container_type.capabilities unfortunately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@EribertoLopez In my other feature branch I added an is_reservoir attribute to the container_type dictionary with default of false to be set to true when writing client protocols. This approach above is per Vanessa's suggestion: https://github.com/autoprotocol/autoprotocol-python/pull/299#discussion_r621251956

In light of these two approaches, do you have an alternative idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And thanks for the feedback!

Copy link
Contributor

@joshhacksthings joshhacksthings left a comment

Choose a reason for hiding this comment

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

left comments. lookin' good 😎

@@ -6,7 +6,7 @@
:license: BSD, see LICENSE for more details

"""

# fmt: off
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you turning off the Black formatter here? Don't like it? 🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh whoops, I'll fix that haha

Comment on lines 1 to 2
# fmt: off
# pylint: disable=W0703
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you turning black and pylint off?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🙈

test/util_test.py Show resolved Hide resolved
Copy link
Contributor

@joshhacksthings joshhacksthings left a comment

Choose a reason for hiding this comment

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

i think i misunderstood your tests initially...

see my comment/question and lmk if what im asking makes sense

test/util_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@joshhacksthings joshhacksthings left a comment

Choose a reason for hiding this comment

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

looks good! great test coverage 🚀

@aaronkarlsberg1 aaronkarlsberg1 merged commit d5d5cb8 into master May 25, 2021
@aaronkarlsberg1 aaronkarlsberg1 deleted the allow-384-well-reservoir-stamping branch May 25, 2021 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants