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 name field to pipette v2 general specs #15251

Open
wants to merge 5 commits into
base: chore_release-8.0.0
Choose a base branch
from

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented May 23, 2024

Overview

We need a to add name to v2 of pipette specs under general since it isn't otherwise represented in this version of the schema. The name for each pipette is pulled from the key of pipetteNameSpecs and the name field of pipetteModelSpecs

Test Plan

Changelog

Review requests

Risk assessment

@smb2268 smb2268 requested review from sfoster1 and jerader May 23, 2024 14:45
@smb2268 smb2268 requested review from a team as code owners May 23, 2024 14:45
Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.36%. Comparing base (ccc944e) to head (9fabef6).
Report is 2 commits behind head on edge.

Current head 9fabef6 differs from pull request most recent head 721668a

Please upload reports for the commit 721668a to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #15251       +/-   ##
===========================================
- Coverage   92.43%   81.36%   -11.08%     
===========================================
  Files          77      119       +42     
  Lines        1283     4100     +2817     
===========================================
+ Hits         1186     3336     +2150     
- Misses         97      764      +667     
Flag Coverage Δ
shared-data 76.32% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 42 files with indirect coverage changes

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

nice, lgtm! Might be good to add a quick pytest for this though - maybe similarly to test_model_enum in shared-data/python/tests/pipette/test_pipette_definition.py but instead of testing a model, it tests that each name equals a PipetteNameType (just a suggestion, i'd defer to @sfoster1's recommendations on test coverage 😄)

@sfoster1
Copy link
Member

nice, lgtm! Might be good to add a quick pytest for this though - maybe similarly to test_model_enum in shared-data/python/tests/pipette/test_pipette_definition.py but instead of testing a model, it tests that each name equals a PipetteNameType (just a suggestion, i'd defer to @sfoster1's recommendations on test coverage 😄)

Yeah, I agree

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

I agree with @jerader 's point about adding a test, but looking at this in practice I'm a little worried that we're still implicitly relying on data from v1 of the pipette definitions - specifically the PipetteName type.

Can tsc load type data from jsonschema? If so, maybe the right thing to do here is actually for this to be an enum in the schema so tsc can pick it up there. That ability would be enough for me to want to encode it in the schema instead of implicitly, even though I'd previously made the point that we should do that.

@smb2268 smb2268 changed the base branch from edge to chore_release-8.0.0 September 3, 2024 18:01
@smb2268 smb2268 force-pushed the shared-data_pipette-v2-name branch from 721668a to 7c2e096 Compare September 4, 2024 16:27
@y3rsh
Copy link
Member

y3rsh commented Sep 18, 2024

I think the plan for this is a retarget at edge, it will not be in 8.0.0

@y3rsh y3rsh added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants