- Notifications
You must be signed in to change notification settings - Fork689
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Conflicts:dist/alasql-worker.jsdist/alasql-worker.min.jsdist/alasql.fs.jsdist/alasql.jsdist/alasql.min.js
codecov-io commentedMar 22, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 oftest/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?
kanghj commentedMar 23, 2017
@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? |
mathiasrw commentedMar 23, 2017
Of cause... I suggest you do a and make a test that starts with |
Conflicts:dist/alasql-worker.jsdist/alasql-worker.min.jsdist/alasql.fs.jsdist/alasql.jsdist/alasql.min.js
kanghj commentedMar 26, 2017
@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. |
mathiasrw commentedMar 26, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Really nice work. Let me know when to merge the PR. |
mathiasrw commentedApr 6, 2017
Any news on this? |
kanghj commentedApr 6, 2017
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.
Uh oh!
There was an error while loading.Please reload this page.
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