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

Various problems with options types #219

Closed
pixelzoom opened this issue Mar 3, 2024 · 2 comments
Closed

Various problems with options types #219

pixelzoom opened this issue Mar 3, 2024 · 2 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 3, 2024

For code review #32 ...

In addition to problems noted in #216, there are a whole bunch of problems related to options types, including...

  • type names that don't match their associated class, e.g. MeasuresScreen and ProjectileDataLabScreenOptions
  • parent options type that does not match the superclass
  • swapped parameters to optionize (<Parent,Self,Provided> instead of <Provided,Self,Parent>)
  • missing uses of WithRequired<..., 'tandem'>
  • redundant usees of WithRequired<..., 'tandem'>
  • etc.

It will be easier for me to fix these than to enumerate them.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 3, 2024

I fixed many problems in d01f7e2. There are probably more. Over to @samreid or @matthew-blackman to review.

Note that I did not do any narrowing of options types, nor will I open an issue about it, because I know that @samreid and I disagree on that topic. But I will note that this sim generally has options types that are unnecessarily broad, and sometimes dangerously broad -- allowing a client to blast something that is the responsibility of the associated class, and should not be part of the class' options API.

@matthew-blackman
Copy link
Contributor

I went through each usage of optionize and checked that the correct options patterns were being applied. I caught 2 more cases that are fixed in the commits. I don't see any further work to be done here. Feel free to reopen if more errant options types pop up. Closing.

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

3 participants