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

Avoid string allocations in DateField / DecimalField / TimeField when… #31

Merged
merged 3 commits into from Feb 10, 2022

Conversation

campersau
Copy link
Contributor

@campersau campersau commented May 14, 2021

… targeting netstandard2.1

Also removes Regex parsing. Was there a specific reason to use it instead of date/time format strings?


There is a new LogFilePrefix for vstest so the trx files for the different TargetFrameworks don't override each other.
https://github.com/microsoft/vstest-docs/pull/204/files?short_path=3562af9#diff-3562af9d9cfa496e16a73dfbcf906d7330a8d5a16a53c17df4e7c4b0f18901ac
microsoft/vstest#2140
microsoft/vstest#1603

@codecov-commenter
Copy link

Codecov Report

Merging #31 (01d0847) into master (92e64e9) will decrease coverage by 0.84%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   83.75%   82.91%   -0.85%     
==========================================
  Files          32       32              
  Lines         942      948       +6     
  Branches      135      135              
==========================================
- Hits          789      786       -3     
- Misses        150      159       +9     
  Partials        3        3              
Impacted Files Coverage Δ
src/SapNwRfc/Internal/Fields/TimeField.cs 87.09% <81.81%> (-12.91%) ⬇️
src/SapNwRfc/Internal/Fields/DateField.cs 87.09% <90.00%> (-12.91%) ⬇️
src/SapNwRfc/Internal/Fields/DecimalField.cs 96.96% <100.00%> (-3.04%) ⬇️

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 92e64e9...01d0847. Read the comment docs.

@huysentruitw
Copy link
Owner

The date parsing looks like an improvement at this point. However, I would prefer not to have different code between netstandard2.0 and netstandard2.1. Using Spans is cool but will not improve performance much for this library as SAP itself is slow as hell in my experience.😅

Also, if we would like to go the Span route, I would than prefer to drop the netstandard2.0 target.

@campersau campersau changed the base branch from master to develop February 10, 2022 08:56
@campersau
Copy link
Contributor Author

Okay, I removed the spans. Why is there a netstandard2.1 target then? 😄

@huysentruitw
Copy link
Owner

Okay, I removed the spans. Why is there a netstandard2.1 target then? 😄

That's a very good question 🤔

@huysentruitw huysentruitw merged commit 29619d5 into huysentruitw:develop Feb 10, 2022
@campersau campersau deleted the spanify branch February 10, 2022 09:23
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

3 participants