-
Notifications
You must be signed in to change notification settings - Fork 513
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
Refactors dicts to mimic python behavior #41
Conversation
Conflicts: src/brython.js src/py2js.js src/py_VFS.js
This Pull Request needs a Re-Sync with the Master (conflicts) 🔍 |
Just out of curiosity, where are we on this? The issue was marked as in-progress so it isn't clear if there was already work done to get this merged in or not. I know I was having issues getting the unit tests set up on my machine and just didn't have the spare time to work through that. |
Regarding getting unit tests to run on one's local machine - one of the things I'm working on (which is blocked by #121) is a test runner that uses Phantom JS; if Brython itself used this, it would make it possible to run tests on a service integrated with Github, such as Travis CI. |
Glyph, Can you provide some of the code you are using with Phantom JS? I would to take a look. |
yeah I know / have seen. but I thought this PR / change was WIP or still waiting to be merged, so my comment, but apparently it's not so. |
This changes the behavior of dicts to mimic python's behavior. Dicts now have their own data array consisting of key/value pairs. Objects are added to an index based on a hash value, and the array will grow when it is 75% full. Items are deleted by replacing them with a dummy object. This results in significantly faster look-ups, and also adds meaning to the hash function.
Unfortunately, a lot of existing code was coupled with the previous implementation, so the risk of introducing bugs is high. This is exasperated by the fact that I can't seem to get the unit test suite to run properly on my local machine, so there will likely be some clean-up / additional bug fixing required after merging this.