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

Remove unnecessary operation when sorting users #193

Merged
merged 1 commit into from Mar 15, 2016

Conversation

astorije
Copy link
Member

Maybe there was a good reason for this .reverse() but I can't admit I'm seeing it right now...
The entire method could probably use a rewrite but this is the least that can be done here.

@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Mar 14, 2016
@xPaw
Copy link
Member

xPaw commented Mar 14, 2016

I wouldn't merge this because #167 actually knows which user modes are used on the server, and that can be used instead of this hardcoded array.

@xPaw
Copy link
Member

xPaw commented Mar 14, 2016

Okay, I did it in 53c367b, and now it sorts the whole user list within a single pass. Closing this PR.

@xPaw xPaw closed this Mar 14, 2016
@astorije astorije reopened this Mar 14, 2016
@astorije
Copy link
Member Author

Reopening this: even though it's already done in #167, it is still a few weeks down the road (that kind of change requires intensive testing and is not ready yet, not to mention reviewers' time allocation) while this PR is already there and done.
If others think it's unnecessary so be it, but I think this is a minor improvement that can easily go with the next patch release.

@AlMcKinlay
Copy link
Member

It's obviously not a massive improvement (although it is an improvement none-the-less) so I wouldn't have spent the time looking for it. However, as you have already done the work, I see no reason to deny it, especially as #167 won't be done anytime soon (we are at the very least waiting on 2 bugs to be fixed with the framework itself).

So I'll 👍 this but stick second reviewer on it, and @maxpoulin64 or @xPaw can fight it out :-P

@maxpoulin64
Copy link
Member

I guess I'll side with @YaManicKill on this one, since it is a really trivial fix and 👍. It's kind of useless even for me as this really only moves 4 values, but I don't see any particular reason to deny it. I'm looking forward to @xPaw 's actual proper implementation of course.

maxpoulin64 added a commit that referenced this pull request Mar 15, 2016
Remove unnecessary operation when sorting users
@maxpoulin64 maxpoulin64 merged commit e54409b into master Mar 15, 2016
@maxpoulin64 maxpoulin64 deleted the astorije/minor-simplification branch March 15, 2016 00:55
@astorije astorije added this to the 1.4.0 milestone Oct 8, 2016
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants