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

Add --reverse option #51

Closed
wants to merge 4 commits into from
Closed

Add --reverse option #51

wants to merge 4 commits into from

Conversation

jgberry
Copy link
Collaborator

@jgberry jgberry commented Mar 15, 2022

A proof of concept for a --reverse option as mentioned in #18. There is still some more work to be done here, but I wanted to open a draft to foster discussion on the approach I'm using here. Surprisingly, it seems this is a fairly straightforward addition.

@jgberry jgberry requested a review from bwhmather March 15, 2022 02:27
@jgberry
Copy link
Collaborator Author

jgberry commented Mar 15, 2022

One caveat with this feature is that it may create unexpected orderings of global variables. For instance, it is common practice to declare module constants and global variables at the top of a file. However, when using the --reverse option these declarations tend to drift downwards so that they are declared after their usages.

Imports can encounter a similar issue as mentioned in #18.

@jgberry jgberry linked an issue Mar 15, 2022 that may be closed by this pull request
@jgberry jgberry self-assigned this Mar 15, 2022
@bwhmather
Copy link
Owner

I should warn you ahead of the time that the bar for this being merged is quite high. ssort was started as a reaction to others trying and repeatedly failing to manually sort modules in reverse order in a consistent way.

There is also the question of whether we want to risk splitting the ecosystem. There is something to be said for the any-colour-you-like approach.

That said, copy the samples tests and let's see what the output looks like on real code.

@jgberry
Copy link
Collaborator Author

jgberry commented Mar 15, 2022

Ok, given your insights and some quirks I discovered, which I captured in failing unit tests in the above commits, I think it would be best to put this work on hold for now.

@jgberry
Copy link
Collaborator Author

jgberry commented Mar 17, 2022

Closing this draft for now.

@jgberry jgberry closed this Mar 17, 2022
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.

Feature Request: Reverse topological order / top down reading order
2 participants