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

Do not change visibility when sorting #142

Merged
merged 1 commit into from Jan 10, 2023
Merged

Conversation

mwudka
Copy link
Contributor

@mwudka mwudka commented Dec 23, 2022

Hopefully fixes #141 . Updates the SortNodes visitor to preserve visibility of nodes. So, a file that looks like

class MyClass
def zethod2; end
def zethod1; end
private
def c; end
def b; end
end

will get sorted as

class MyClass
def zethod1; end
def zethod2; end
private
def b; end
def c; end
end

rather than as

class MyClass
def b; end
def c; end
private
def zethod1; end
def zethod2; end
end

@mwudka mwudka requested a review from a team as a code owner December 23, 2022 01:53
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

Thanks!

I like the 2 pass solution. Although it was hard to understand at first, it may be useful to extract the core sorting logic to its own method (L37-L46). What do you think?

test/rbi/rewriters/sort_nodes_test.rb Show resolved Hide resolved
@paracycle
Copy link
Member

I think there is a better way to do this by using partition and sorting each of the partitions independently. If we did that, we wouldn't have to do all the manual offset calculations that are present in this PR.

The SortNodes visitor also sorts Private and Protected nodes. This can cause
the visibility of methods to change. For example, a private method called
"a_thing" will always be moved before the private specifier, thus becoming
public. This change extends the SortNodes visitor to individually sort the
public, private, and protected nodes to preserve their visibility.
@mwudka
Copy link
Contributor Author

mwudka commented Jan 9, 2023

After looking into partition, I realized that my original approach was wrong: visibility modifiers can be repeated, so there can be an arbitrary number of chunks. The helpful partition suggestion from @paracycle got me looking into other ruby array methods I don't know, and I found chunk which I think makes things much clearer.

It does increase allocations, but perhaps that is a good tradeoff here, since this isn't runtime code?

@KaanOzkan the code is now much shorter, so perhaps the helper method is not needed? I am on the fence, but leaned towards keeping it inline so that the flow is clear and matches the existing structure more closely.

@mwudka mwudka requested a review from KaanOzkan January 9, 2023 23:08
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

This is definitely simpler thank you!

@KaanOzkan KaanOzkan added the bugfix Fix a bug label Jan 10, 2023
@KaanOzkan KaanOzkan merged commit 671efc8 into Shopify:main Jan 10, 2023
@shopify-shipit shopify-shipit bot temporarily deployed to production July 10, 2023 20:39 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sort_nodes! changes method privacy
3 participants