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

Properties should use type-specific subclasses where appropriate (e.g. BooleanProperty, NumberProperty, StringProperty) or provide documentation as to why they are not. #46

Closed
Tracked by #5
samreid opened this issue Aug 11, 2021 · 5 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Aug 11, 2021

For #5, I saw several places that could use EnumerationProperty, and some that could use Vector2Property.

@jonathanolson
Copy link
Contributor

I just fixed up what I was able to find. I definitely didn't see any place where EnumerationProperty could be used that isn't, could you clarify?

Additionally, should we open up a common code issue about Vector3Property, Bounds3Property, and other things not existing that I would have expected would have been kept in-line with the other dimensional equivalents?

@samreid
Copy link
Member Author

samreid commented Aug 11, 2021

Could EnumerationProperty be used in cases like these:

this.interiorMaterialProperty = new Property( BOTTLE_INITIAL_INTERIOR_MATERIAL );

new Property( Material.WOOD )

new Property( Gravity.EARTH, {

etc?

@samreid
Copy link
Member Author

samreid commented Aug 11, 2021

Additionally, should we open up a common code issue about Vector3Property, Bounds3Property, and other things not existing that I would have expected would have been kept in-line with the other dimensional equivalents?

Perhaps phetsims/axon#221 was intended for that? I'm OK with any of these plans:

  • create an issue for making those
  • create those
  • just use the ones that exist and not worry about the others at the moment.

What do you recommend?

@jonathanolson
Copy link
Contributor

Could EnumerationProperty be used in cases like these

Not that I'm aware of, custom gravities and materials are created dynamically and seem unsuitable to Enumeration. If there's a way to provide better phet-io support, let me know.

Perhaps phetsims/axon#221 was intended for that? I'm OK with any of these plans:

create an issue for making those
create those
just use the ones that exist and not worry about the others at the moment.

What do you recommend?

I'd defer to your judgment, I'm fine to not make changes.

@samreid
Copy link
Member Author

samreid commented Aug 11, 2021

Thanks, I forgot about the custom ones. All seems well, closing.

@samreid samreid closed this as completed Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants