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

Rewrite selectors.py and add more tests #307

Merged
merged 2 commits into from
Feb 14, 2016

Conversation

smallnamespace
Copy link
Collaborator

Rewrite selectors completely:

  • Get rid of the 'Forth-like' language in favor of representing selector DSL nodes directly as Python objects
  • No need to linearize the AST (which is what the Merge, Unmerge, and MergeFilter ops essentially did)
  • Python program counter and stack are used instead of an explicitly managed stack / PC, which is faster
  • I benchmarked a 10-20% performance improvement in full game runs, will profile selector performance explicitly later

@smallnamespace
Copy link
Collaborator Author

@mischanix
Would appreciate your comments

@smallnamespace
Copy link
Collaborator Author

Profiling runs at #302

@jleclanche
Copy link
Owner

If you do a first commit that creates a dummy class EnumSelector, child of Selector, and move all the enum selectors to it, this will be easier to review :)

@smallnamespace
Copy link
Collaborator Author

@jleclanche

I went ahead and rewrote the history like you suggested. Btw, my initial version of this used the typing module from 3.5, and there is a backport available. If you're ok with it, we can either:

  1. Move Python requirement to 3.5 (would require updating Travis)
  2. Make a hard dependency on the backport in requirements.txt (probably not ideal going forward, since it always will pull from PyPI even when unneeded)
  3. or migrate requirements.txt to setup.py instead, so we can version-conditional dependencies

I would prefer 3 since it's ultimately cleanest while not moving up requirements and potentially breaking people -- for example, Ubuntu trusty only goes up to 3.4 by default.

@smallnamespace smallnamespace force-pushed the new_selectors branch 2 times, most recently from 8ba3ee1 to 35fdcd6 Compare February 12, 2016 16:40
@smallnamespace
Copy link
Collaborator Author

Split out AttrSelector -> AttrValue as requested

@robert-nix
Copy link
Collaborator

Looks good to me.

@smallnamespace smallnamespace force-pushed the new_selectors branch 5 times, most recently from e9189a9 to f52a234 Compare February 13, 2016 06:15
@jleclanche
Copy link
Owner

Typing annotations: Why are most of them strings of objects?

@jleclanche
Copy link
Owner

Overall code looks good, mostly just minor styling issues. Thanks. This looks like a really great rewrite.

@smallnamespace
Copy link
Collaborator Author

Most of the typing annotations were changed once I pulled in the py3.5 commit.

The remaining ones which are strings are forward references so type hints on a class's methods can refer to the class type itself (which hasn't been constructed yet).

@jleclanche
Copy link
Owner

Landed your first 2 commits. Here's what's left to do:

  1. Add an autoclose to the 3.5 commit
  2. Sync your 3.5 PR with that commit so that it closes when I land this
  3. Take care of the last remaining comments

@smallnamespace
Copy link
Collaborator Author

Done

@jleclanche jleclanche merged commit 5316f18 into jleclanche:master Feb 14, 2016
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.

3 participants