Skip to content

Add quantity using codegen #892

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

Closed

Conversation

generateui
Copy link
Contributor

Proof of concept pull request enabling client libraries to define their own quantities in .json files. They can then use the code-generation functionality (modified in this PR) of UnitsNet to generate their own quantities, units and quantity factory.

This PR is initiated to solve a real problem at one of my clients. The usecase is software specific to a certain industry, where the industry in question uses many specialized quantities. Not only that, the industry uses common quantities as well. It would be great if UnitsNet can be used to support both cases: by default, use the common quantities with their units while allowing extensibility via uncommon or specialized quantities.

Since this PR may be a bit abstract and confusing since it mostly modifies code-generation code and publishes internal types, I have created a simple example using two quantities on generateui/UnitsNetQuantityExtensionExample. It uses the fork of this PR as a submodule for convenience of development.

The purpose of this PR is to gather feedback of the desirability of this implementation. Secondly, it wants to explore ideas on how to actually implement quantity extensibility (adding a quantity by a client library).

Note the following:

  • partial codegen is used (tests are left out for now, may be added later)
  • jerk (aka jolt) as quantity is chosen as a quantity example. I picked an example quantity which is understandable and not too specialized.
  • depth as example is picked as a future example for potential support of derived units (out of scope for this PR)
  • Allowing client libraries to support .json files enables easy promotion to UnitsNet. Is this desirable?
  • since Name of quantity within the .json is used as a C# symbol name, a potential good addition is to add a field in .json called SymbolName to differentiate between a C# symbol name and an actual name (which then has less strict naming character restrictions)
  • I'm very aware that the actual changes in this PR are "rough" and can definitely get some polishing. Please ignore the here-and-there ugly implementation details, please focus on the concept.
  • Since generated code uses some internal helper functions, they are made public. I feel at some places this seems OK at first sight, others seems much less so.

@angularsen
Copy link
Owner

Sounds awesome, I love the idea!

I have a hard time finding the time needed to go into depth of reviews though, so I can't give you any constructive feedback right now.

It seems like there is a bit work in progress still, do you plan to iterate more on it on your own or are you primarily waiting for a review by now?

@rohahn
Copy link
Contributor

rohahn commented Jan 19, 2021

I had a similar issue yesterday when I tried to integrate our new quantity, before it was merged into UnitsNet, by copying the generated classes to our codebase. The first issue I tried to fix with a smaller PR is the QuantityInfo initialization. The next would be those internal helper classes. Maybe we can approach this with several smaller PR that are easy to review?

@generateui
Copy link
Contributor Author

generateui commented Jan 21, 2021

Sounds awesome, I love the idea!

👍

I want to validate the idea and some implementation directions. I guess the most important hurdle is taken, support from you/user(s), which is great :). I like to proof the pudding first with a small proof-of-concept PR, which is this.

The next step is to get a working PR working towards a mergeable PR.

An important point I did not note yet is that including generation of tests causes code in the test library to become public API. It will change from internal/friend accessibility to public[Api] accessibility. You may think twice of doing so, as it decreases the flexibility the library has in changing test implementation. A sensible solution to me is to move the test library code to a separate test library library (inception), such that flexibility is kept in implementation of the internal UnitsNet tests. This probably also communicates intent better. For the PR, we should probably refrain from it yet but slowly refactor over time, all the while a big warning sign is above the generated tests "experimental feature".

Maybe we can approach this with several smaller PR that are easy to review?

You're welcome to slice of parts of this PR and submit them as separate PRs if that fits your usecase.

@stale
Copy link

stale bot commented Jun 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 30, 2021
@stale stale bot closed this Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants