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

Add support of metric numeral expression. fixes #456 #459

Closed
wants to merge 10 commits into from
Closed

Add support of metric numeral expression. fixes #456 #459

wants to merge 10 commits into from

Conversation

aloisdg
Copy link
Contributor

@aloisdg aloisdg commented Sep 20, 2015

Hello,

I created the PR because this is a full working draft. I would be glad to get any kind of reviews.
This is my first time with xUnit (coming from NUnit and MsTest). Any help is welcome!

Features:

  • Convert from metric numeral expression (double only)
  • Convert to metric numeral expression (double only)
  • Add int support
  • Support all symbols (decimal include)
  • Support the name of each symbols (see List of SI prefixes) (partial => export is supported)
  • Support the name of each symbols (see List of SI prefixes) (partial => import is supported)
  • Handle space between number and symbol
  • Add ByteSize's similar syntax
  • Add the choice of precision
  • Add the support of small symbol / name (this include a switch from char to string in symbols because of deca "da"). Full list : { hecto, h, 1E+2 }, { deca, da, 1E+1 }, { deci, d, 1E-1 }, { centi, c, 1E-2 }. This feature will be lock by default. A boolean in parameter will unlock it.

Also:

  • Up-to-date and complete XML documentation
  • Include unit tests
  • Include unit tests with exception coverage

cheers,

@aloisdg
Copy link
Contributor Author

aloisdg commented Sep 21, 2015

I don't know if I want to switch or to add ByteSize Syntax. I created this extensions with a syntax near the one from RomanNumeral in mind. Should I do the move? Is it worth it?

If I switch to the OOP, it will allow devs to use operator overloading for example. If It would be nice to have the possibility to use it, I think it is better to use only the basic double number and to keep this methods only for I/O with the user. Any counter-example?

@aloisdg
Copy link
Contributor Author

aloisdg commented Sep 21, 2015

For the choice of precision, I am thinking about two ways:

  • Full format support:
public static string ToMetric(this double input, string format,
    bool isSplitedBySpace = false, bool useSymbol = true)
{
    // ...

    return number.ToString(format)
        + (isSplitedBySpace ? " " : String.Empty)
        + GetUnit(symbol, useSymbol);               
}
  • Precision only support:
public static string ToMetric(this double input, int n,
    bool isSplitedBySpace = false, bool useSymbol = true)
{
    // ...

    return number.ToString("#." + new string('#', n); // n is the count of numbers
        + (isSplitedBySpace ? " " : String.Empty)
        + GetUnit(symbol, useSymbol);               
}

Also

Why not both

@aloisdg
Copy link
Contributor Author

aloisdg commented Sep 21, 2015

I think about switching from optional parameters to using overloads methods. I find it easiest to deal with this in public API. Is there any guidance around this in Humanizer? By the way, is there a StyleCop file (or alternative) for the project?

more reading : 1 2

@clairernovotny
Copy link
Member

One favor -- Would you be able to rebase this onto the dev branch? If at all possible, it'd a big help.

How's this looking from a completion perspective otherwise, is it ready for review?

Thanks!

@clairernovotny
Copy link
Member

Actually, nm...I'll handle the merge into dev when you're ready. Don't worry about it. Just let me know when you're ready. Thanks!

@aloisdg
Copy link
Contributor Author

aloisdg commented Oct 25, 2015

Ok thank you for the help. For now, everything works (AFAIk). I think I will let the support of e < 3 open. If someone want it, just create it. For the two others unchecked features, I would love some advices. I think it is a matter of preference.

@clairernovotny
Copy link
Member

I've rebased this PR onto https://github.com/MehdiK/Humanizer/tree/aloisdg-metric for review

@aloisdg
Copy link
Contributor Author

aloisdg commented Oct 25, 2015

Nice. Thanks.

@clairernovotny
Copy link
Member

I've merged this in to the dev branch. Thanks for your contribution!

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

Successfully merging this pull request may close these issues.

2 participants