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

V1.10.x and stealth add v2 features #267

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

V1.10.x and stealth add v2 features #267

wants to merge 33 commits into from

Conversation

tilo
Copy link
Owner

@tilo tilo commented Jan 1, 2024

Adding Experimental v2 Features 🧪 🌶️

This PR adds the features from the 2.0-develop branch to the main branch,
but as hidden / experimental features.

The intent is that users can try out the new behavior and send feedback / contribute PRs.

When option v2_mode: true is set, the new code paths are used.

Still missing in this PR / TO DO before merging:

  • need to update the options processing to have default values in v2_mode that make it behave like v1.x

@tilo tilo mentioned this pull request Jan 8, 2024
@mscrivo
Copy link

mscrivo commented Jan 8, 2024

Thanks @tilo! A couple of questions regarding this (I have not yet looked at the changes):

  1. Is the intent that if we were using 2.0-pre, that if we switch to this and set the flag, our code should work without any other changes?
  2. Curious, what's the longer term plan? Are you thinking this this flag is going to be the final form of the 2.0 functionality and it just gets merged into 1.x branch? Or are you planning on making a 2.0 breaking change release that doesn't carry the 1.x baggage at some point?

@tilo
Copy link
Owner Author

tilo commented Jan 8, 2024

Thanks @tilo! A couple of questions regarding this (I have not yet looked at the changes):

1. Is the intent that if we were using 2.0-pre, that if we switch to this and set the flag, our code should work without any other changes?

2. Curious, what's the longer term plan? Are you thinking this this flag is going to be the final form of the 2.0 functionality and it just gets merged into 1.x branch? Or are you planning on making a 2.0 breaking change release that doesn't carry the 1.x baggage at some point?

@mscrivo
The 2.0-develop branch is quite old, and there were many updates on the main branch since then - so the easiest way to consolidate the two versions was to refactor the 1.x code and to pull select changes into the main branch.

This PR is still WIP, so I'm not making promises to be 100% compatible with 2.0-pre, but I'll try.

Because the change from v1.x to v2.x could be disruptive to users, I decided to add both code versions to main for now, and switch them via the v2_mode option. In v1.x versions this will default to false, and in future v2.x versions it will default to true. The idea is that the user can switch over to the new behavior on a more flexible schedule.

Eventually the 1.x code will be deprecated and removed.

@tilo tilo added the v2.0 label Jan 12, 2024
@tilo
Copy link
Owner Author

tilo commented Jan 14, 2024

with old count_quote_cards:

time: 10.101 ms

allocated memory by class

358020  String
112656  Hash
 79656  Array
 54096  MatchData
 44280  Regexp
  8432  File

allocated objects by gem

 11030  smarter_csv-1.11.0.pre1

allocated objects by class

  8657  String
  1789  Array
   322  MatchData
   171  Hash
    90  Regexp
     1  File     

with new count_quote_chars:

time: 6.043 ms

allocated memory by class

151965  String
112656  Hash
 85720  Regexp
 82856  Array
 54096  MatchData
  8432  File

allocated objects by gem

  6176  smarter_csv-1.11.0.pre2

allocated objects by class

  3643  String
  1869  Array
   322  MatchData
   171  Hash
   170  Regexp
     1  File

@tilo
Copy link
Owner Author

tilo commented Jan 26, 2024

  • the new code needs test coverage
  • more performance testing

Copy link

codecov bot commented Apr 7, 2024

Codecov Report

Attention: Patch coverage is 86.30952% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 95.68%. Comparing base (2f264ca) to head (1dc31d5).
Report is 5 commits behind head on main.

❗ Current head 1dc31d5 differs from pull request most recent head eb30e70. Consider uploading reports for the commit eb30e70 to get more accurate results

Files Patch % Lines
lib/smarter_csv/hash_transformations.rb 58.82% 21 Missing ⚠️
lib/smarter_csv/options_processing.rb 94.28% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #267      +/-   ##
===========================================
- Coverage   100.00%   95.68%   -4.32%     
===========================================
  Files           11       11              
  Lines          380      533     +153     
===========================================
+ Hits           380      510     +130     
- Misses           0       23      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

2 participants