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 PrimitivesSetField and deprecate SetOfPrimitivesField #6087

Merged
merged 2 commits into from Jul 10, 2018

Conversation

Projects
None yet
5 participants
@stuhood
Copy link
Member

stuhood commented Jul 10, 2018

Problem

Using SetOfPrimitivesField, it is not currently possible to default a field to None, which is important in situations where you need to differentiate "not set" from "empty".

Solution

Add PrimitivesSetField which allows a field to default to None, and deprecate SetOfPrimitivesField.

@stuhood stuhood requested review from benjyw , alanbato and cosmicexplorer Jul 10, 2018

@alanbato
Copy link
Member

alanbato left a comment

Looks good to me :)
Although I have a question,
Wouldn't adding a parameter like preverve_none=True to the SOPF constructor make it for a better adoption of the new feature?
This way we can preserve current behavior without deprecations and less changes while also allowing for new use cases to be developed with None defaults.
What do you think?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 10, 2018

This behavior better aligns with PrimitiveField, which also preserves None... and I don't think this class is widely used (I updated all usages in the pants codebase in this PR), so the deprecation isn't expensive.

@benjyw

benjyw approved these changes Jul 10, 2018

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

Is the default-None behavior going to be used in some future PR? It looks like these all default to the empty list.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 10, 2018

Is the default-None behavior going to be used in some future PR? It looks like these all default to the empty list.

Yea, sorry: @alanbato will be using this in #6065.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good. I'd like there to be an assertion for the empty vs none case though.

self.assertNotEqual(
sopf(None),
sopf(['one']),
)

This comment has been minimized.

@baroquebobcat

baroquebobcat Jul 10, 2018

Contributor

Could you add an explicit assertion for differentiating "not set" from "empty" to these?

self.assertNotEqual(
  sopf(None),
  sopf([]),
)

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

Hah, good call.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 10, 2018

The last commit adds that test, which I've confirmed-working locally. Have canceled its travis run: will merge when the second to last travis run goes green.

@stuhood stuhood merged commit 26f1d42 into pantsbuild:master Jul 10, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@stuhood stuhood deleted the twitter:stuhood/primitives-set-field branch Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment