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

Various fixes/improvements. #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PJB3005
Copy link

@PJB3005 PJB3005 commented Feb 22, 2021

Sorry for this not being separate PRs, I initially wanted to fix more about the library and figured all the fixes would interconflict anyways so this would be saner. Then I realized the problem is that this library is built on an ancient pre-1.0 version of fluent.js that itself was a complete trash fire code wise so I kind of gave up midway fixing more.

Frankly this library would be better served being re-ported from the current TypeScript or Rust implementations since dear god this old JS implementation is awful but I don't really have the motivation to do such an undertaking, so...

This PR still contains some useful fixes/improvements though that I could use so I figured I'd still submit it.

The individual commits have the individual changes, so look at those.

Should be no backwards compatibility breakages (there is an [Obsolete] now though).

1. It wasn't public which seemed like a mistake.
2. It previously returned IEnumerator<KVPair<...>> which makes no sense. I upgraded it to IReadOnlyDictionary since there's no reason not to and this is more useful.
This file had no business containing all these random types.
This means that if the user passes in a single locale like `en-US` plural handling won't break.

Also allows users to pass custom CultureInfo objects that can have custom formatting parameters (or, for example, disable user system overrides)

Yes I did this because the tests failed on my system due to that last point.
@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #9 (b790b86) into master (3fb1005) will increase coverage by 0.09%.
The diff coverage is 93.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
+ Coverage   93.61%   93.70%   +0.09%     
==========================================
  Files          26       30       +4     
  Lines        1941     1890      -51     
==========================================
- Hits         1817     1771      -46     
+ Misses        124      119       -5     
Impacted Files Coverage Δ
Fluent.Net/BuiltIns.cs 60.00% <60.00%> (ø)
Fluent.Net/Resolver.cs 88.96% <84.21%> (+1.37%) ⬆️
Fluent.Net/Plural/LocaleRules.cs 90.90% <87.50%> (-5.76%) ⬇️
Fluent.Net/MessageContext.cs 90.69% <92.30%> (-3.55%) ⬇️
Fluent.Net/FluentError.cs 100.00% <100.00%> (ø)
Fluent.Net/FluentResource.cs 100.00% <100.00%> (ø)
Fluent.Net/Types.cs 100.00% <100.00%> (ø)
Fluent.Net/ParserStream.cs 85.56% <0.00%> (-0.71%) ⬇️
Fluent.Net/Serializer.cs 91.51% <0.00%> (-0.15%) ⬇️
Fluent.Net/FtlParserStream.cs 91.80% <0.00%> (-0.09%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fb1005...b790b86. Read the comment docs.

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.

1 participant