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

Start sorting DSL generators by class name #478

Merged
merged 3 commits into from
Sep 1, 2021
Merged

Conversation

paracycle
Copy link
Member

Motivation

DSL generation runs are currently not repeatable since we don't load DSL generators in any specific order, but the generators could add order dependent items to the constants they decorate, like include calls. Thus, depending on what order generators run, we end up getting a different order of includes which breaks all assumptions of determinism from the generator.

This didn't used to be a problem since Parlour used to sort include calls in the output, so the output ended up being the same regardless of the order the includes were added. However, sorting includes is a bad idea in general, since the behaviour of the resulting constant could be drastically different so the RBI gem does not do that.

Implementation

We started sorting DSL generators by class name for now. Since we don't have dependencies between generators yet, this simple sorting should be enough and safe. In the future, if we add dependencies between generators, we can replace this with some kind of topological sort, like how Bundler does.

Tests

Extended existing DSL CLI test to add 2 in repo fake generators that decorate the Post class with included modules. If runs start being indeterministic again, expected order of includes should fail these tests.

@paracycle paracycle requested review from Morriar and a team September 1, 2021 22:00
@paracycle paracycle merged commit bd10e68 into main Sep 1, 2021
@paracycle paracycle deleted the uk-sort-dsl-generators branch September 1, 2021 22:27
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.

2 participants