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

Refactor suggestions #290

Merged
merged 3 commits into from
May 28, 2022
Merged

Refactor suggestions #290

merged 3 commits into from
May 28, 2022

Conversation

karljj1
Copy link
Collaborator

@karljj1 karljj1 commented May 27, 2022

Hey. 2 very small refactors I added in our version.

  1. Don't recreate the valid chars array each time in Validation.
  2. Added TimeSpanFormatOptionsPresets so that internal use values are not exposed in the public enum. This is particularly important to us as we show the contents of the Enum in a dropdown selector for users and having internal use values would cause confusion.

@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #290 (82d9d01) into main (0419ced) will increase coverage by 0%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #290   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        91     91           
  Lines      3180   3178    -2     
===================================
  Hits       3064   3064           
+ Misses      116    114    -2     
Impacted Files Coverage Δ
...s.Time/Utilities/TimeSpanFormatOptionsConverter.cs 85% <100%> (ø)
...ormat.Extensions.Time/Utilities/TimeSpanUtility.cs 86% <100%> (ø)
src/SmartFormat/Utilities/Validation.cs 100% <100%> (+33%) ⬆️

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 0419ced...82d9d01. Read the comment docs.

Copy link
Member

@axunonb axunonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent

@axunonb axunonb merged commit 39e327d into axuno:main May 28, 2022
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.

None yet

2 participants