-
Notifications
You must be signed in to change notification settings - Fork 402
Nautical Miles Added #33
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
Conversation
Added Nautical Miles to Length and Area
Thanks. I'm on vacation until July 19 but I'll try to review this in the next few days. |
No worries if you don't get to it right now. It will wait for you :) Have a good vacation. On Tuesday, July 8, 2014, Andreas Gullberg Larsen notifications@github.com
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
xml documentation instead of comment. This way intellisense can show this info too.
Could it be named just NauticalMile, or is the international part important to avoid ambiguity? I have not yet researched the different types of nautical miles, but to me with no experience with that unit I would guess one type of nautical mile was maybe considered the standard or most widely used? |
And could it be sufficient to explain in the xmldoc that this is the international variant? Granted that the international unit can be considered the default/standard one. |
I don't see any tests added. Did you follow those steps? |
I'll look into it. On Thu, Jul 10, 2014 at 6:36 AM, Andreas Gullberg Larsen <
|
Disregard that last one. I see the tests now. The upgrade.htm file should not be included. |
Other than that, it looks good! Awaiting your feedback on the questions and the few fixes. |
Did you find out anything? Some good news and some bad news. Inspired by you using VS Express I finally decided to move to a PowerShell and JSON templating system so having Visual Studio is no longer a requirement to add new units, but it's still probably easier than using notepad. Read more here: #34 The bad news, you will have to redo your work in the new templating system. Fortunately it should be much simpler and it would be great for me to get some feedback on any improvement for the the new templating system. I haven't written a step-by-step wiki for it yet, but basically just edit the Length.json file and see how other units are defined, then run the GenerateUnits.ps1 powershell script, then fix the compile error in the test project for a missing property override, similar to the previous steps in the wiki. That should be it really. Look forward to your comments. |
Sounds good. I'll give it a try. There was a family emergency so I haven't On Friday, July 18, 2014, Andreas Gullberg Larsen notifications@github.com
|
Absolutely, take your time. For your reference, I recently added a new class of units in the new template system. Here is the pull request and its commits: |
I just noticed this one still lingering. Do you still intend to give it another go or should I close this? |
Go ahead and close it. On Wed, Oct 22, 2014 at 1:03 AM, Andreas Gullberg Larsen <
|
OK, no problem. |
Added Nautical Miles to Length and Area