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

unit: clean up unit package #90

Closed
6 of 7 tasks
btracey opened this issue Jun 20, 2017 · 6 comments · Fixed by #950
Closed
6 of 7 tasks

unit: clean up unit package #90

btracey opened this issue Jun 20, 2017 · 6 comments · Fixed by #950
Labels
stability affects API stability

Comments

@btracey
Copy link
Member

btracey commented Jun 20, 2017

The unit package has a lot of problems. These should be cleaned up. Here are my suggestions

  • Clean up basic documentation so that it actually matches the code
  • Remove the (incorrect) comment examples and make them test Examples
  • Do not provide so many constants for the units. Not unit.Millimeter, instead just supply a unit.Milli constant, so it can be unit.Milli * unit.Meter. Slightly less pretty, but drastically reduces the API surface and makes it more composable with outside code
  • Redefine the Unit combination methods to match the mat types. For example, Add should take in two arguments and store to the receiver
  • Remove the formatted field from Unit. It introduces unnecessary state
  • Update the autogeneration to use go generate.
  • It seems like the formatting code for unit types needs fixing.
@kortschak
Copy link
Member

I'm not sure about the 4 point here. There is only a concrete Unit type, so it's reasonable to be able to chain things.

@btracey
Copy link
Member Author

btracey commented Jun 26, 2017

Will ponder.

@kortschak
Copy link
Member

What is the plan for the formatted field?

@kortschak
Copy link
Member

I've been thinking more about the formatted field and I think it should stay. It saves potentially large numbers of allocations (one per print). The race that can exist can be fixed by addition of a sync.RWMutex protecting it.

Related to the combination behaviour point. The zero value should be usable for these; if the dimensions field is nil (as opposed to empty), they should be set to the appropriate state.

@btracey
Copy link
Member Author

btracey commented Mar 30, 2019

I think all that's left of this is point 3

@kortschak
Copy link
Member

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stability affects API stability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants