Move all sorting to the modeling store, and then all views preserve the sorting#3398
Merged
robertbrignull merged 3 commits intomainfrom Feb 26, 2024
Merged
Conversation
koesie10
approved these changes
Feb 23, 2024
Member
koesie10
left a comment
There was a problem hiding this comment.
LGTM, but there seem to be a few failing tests dependent on the sorting
| modifiedMethodSignatures: ReadonlySet<string>, | ||
| processedByAutoModelMethods: ReadonlySet<string>, | ||
| ): Method[] { | ||
| function sortMethodsInGroups(methods: readonly Method[], mode: Mode): Method[] { |
Member
There was a problem hiding this comment.
Perhaps name this something like createGroups or groupMethods since we're not actually sorting the methods?
Contributor
Author
There was a problem hiding this comment.
We can't really call it groupMethods because that's also the name of the method in sorting.ts. I've renamed it to createGroups and we can always tweak again in the future if we want to.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to make sorting of methods easier and prevent them from changing sort order and flying around when you make manual changes. Sorting of methods is moved to the modeling store so it happens in one place only, and then it is the job of each view to preserve the ordering of methods it is given.
This gives us a lot more control over what actions trigger a change in method sorting. As of this PR the only actions that should cause methods to change order are:
It also helps reduce some of the code bloat caused by #3382 because we no longer need to pass as much information to all the different views.
Checklist
ready-for-doc-reviewlabel there.