Skip to content

Conversation

eriove
Copy link
Contributor

@eriove eriove commented Dec 4, 2015

As discussed in #89:

I finally got code at a stage were its worth sharing. I've added code generation for overloads. This is what it looks like for Length (sorry about the re-formatting of the Length.json file, VS did that automatically).
´´´
"OperatorOverloads": [
{
"Operator": "/",
"OtherUnit": "TimeSpan",
"ReturnUnit": "Speed",
"ReturnUnitBasePluralName": "MetersPerSecond",
"OtherUnitBasePluralName": "TotalSeconds"
},
´´´
It would be nice if it could automatically create the corresponding overload in Speed (I think all the needed information is there), but since this is the first time I'm working with PowerShell scripts it would be faster for me to do this manually for the 32 quantities...

At the moment I've only added the operator overloading for Length. If you like it I can do the same for the rest of the units.

I've added some tests in InterUnitConversionTests.cs to show the end result but I can't think of good auto-generated test. All tests I can think of would just test the generation code and not the actual conversion. If you have a good idea I'm happy to do the work.

@angularsen
Copy link
Owner

Thanks for the proposed changes, Erik.

I do think, however, that using code generation is unnecessarily complicated to achieve what you want. There is very little code generated from the JSON so you don't really save much time on implementing new changes and I'd argue it is harder to maintain and learn compared to C# code.

How about implementing this directly in CustomCode\Length.extra.cs instead? And similar for any other unit types where these types of overloads make sense. I would also recommend a test case for each implemented overload.

@eriove
Copy link
Contributor Author

eriove commented Dec 6, 2015

When I started with the code generation my idea was that it would be enough to define / or *. Then the other operator (for the other unit) could be automatically defined. Then I got stuck at the PowerShell scripts... I'll give it some more thought, but just typing it in the .extra.cs make sense as it is right now.

@eriove
Copy link
Contributor Author

eriove commented Dec 14, 2015

I agree with you that simply adding the overloads in the .extra.cs files is easier. You can close this pull request. Will open a new one with the operator overloading in the extra files.

@angularsen
Copy link
Owner

Ok cool, thanks.

@angularsen
Copy link
Owner

I'm fine with large pull requests as long as it address a logical grouping of changes, such as adding overload operators to units.

@eriove eriove deleted the feature/additional-overloading branch February 7, 2016 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants