Skip to content

Conversation

@henrymercer
Copy link
Contributor

The CodeToFeatures library is now mostly a relic from older days, and is now unnecessarily complicated, getting in the way of onboarding new people to the codebase and optimizing queries. This PR (along with corresponding internal PRs) replaces it with much simpler definitions for the enclosingFunctionBody and enclosingFunctionName features. These new definitions are behaviour-preserving but much more readable and maintainable.

Commit-by-commit review recommended.

@henrymercer henrymercer requested a review from Z80coder January 12, 2022 13:00
@henrymercer henrymercer requested a review from a team as a code owner January 12, 2022 13:00
@github-actions github-actions bot added the JS label Jan 12, 2022
@henrymercer henrymercer requested review from annarailton and removed request for Z80coder January 12, 2022 17:52
Copy link

@annarailton annarailton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added some comments.

I think I've understood the changes here. Since this is meant to be a "zero diff" PR (i.e. not change any of the final behaviour), if the pipeline comes out with zero / near zero differences with this change in, I'm happy.

The refactoring to remove the `CodeToFeatures` AST reintroduced a
performance problem. This commit resolves it by pushing size
restrictions into intermediate predicates.
@henrymercer henrymercer force-pushed the henrymercer/atm-remove-code-to-features branch from 9a0a660 to 92d6fec Compare January 13, 2022 16:29
Copy link

@annarailton annarailton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comment updates!

@henrymercer henrymercer merged commit ed28b7f into main Jan 14, 2022
@henrymercer henrymercer deleted the henrymercer/atm-remove-code-to-features branch January 14, 2022 15:31
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.

3 participants