-
Notifications
You must be signed in to change notification settings - Fork 689
Autoincrement for local storage #847
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
Conversation
Conflicts: dist/alasql-worker.js dist/alasql-worker.min.js dist/alasql.fs.js dist/alasql.js dist/alasql.min.js
Codecov Report
@@ Coverage Diff @@
## develop #847 +/- ##
===========================================
+ Coverage 67.73% 67.75% +0.01%
===========================================
Files 1 1
Lines 9881 9886 +5
Branches 2949 2949
===========================================
+ Hits 6693 6698 +5
Misses 3188 3188
Continue to review full report at Codecov.
|
mathiasrw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really awesome. Thank you for taking the time to get it done.
We really should make a test that reflects the issue so we know we dont break this feature in the future.
Please make a copy of test/test000.js and rename to next number in the row of tests (613 as I recall) and try to replicate a scenario that this PR fixes?
|
@mathiasrw, I'll probably need help on the tests. This scenario here only affects localstorage, but all the tests seem to be running in a node environment, how do I test local storage specific code? |
|
Of cause... I suggest you do a and make a test that starts with |
Conflicts: dist/alasql-worker.js dist/alasql-worker.min.js dist/alasql.fs.js dist/alasql.js dist/alasql.min.js
|
@mathiasrw, I have modified an existing test to only pass with the modifications added in this PR. Looks like there was support for writing LocalStorage tests in alasql already after all! Very cool. Unfortunately, there are other places where autoincrement for locastorage ought to work, but somehow still isn't (e.g. test389 when AUTOCOMMIT is off), I'll investigate them. |
|
Really nice work. Let me know when to merge the PR. |
|
Any news on this? |
|
Sorry, I got a bit busier than expected and this was a little harder to track down than I thought... Perhaps let's merge this PR first, but not close the issue. I'll work on this sometime next weekend when I have more time. |
Merging as its better to add a fix to part of the problem than to add no fix.
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mathiasrw <[email protected]> Co-authored-by: Mathias Wulff <[email protected]>
http://jsfiddle.net/80smosgp/2/ should show that it works as expected.
This is really @seb-ster 's code provided in the issue thread #462