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

Enable the Action Order cop for remaining controllers #3781

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

gravitystorm
Copy link
Collaborator

Where actions were reordered, the rails standard actions were also moved to the top of each controller.

My personal preference is for index/new/create/edit/update/destroy (i.e. pair off new/create and edit/update) but I guess it's better to follow the default order, which is based on the rails scaffolding order too.

I would be delighted at some point if we get to the point of having only these actions, but that's work for another day. 😃

I've also fixed a comment (by deleting most of it) that I noticed while reordering actions - it referred to the old approach of having multi-purpose actions which was refactored a while ago.

Where actions were reordered, the rails standard actions were
also moved to the top of each controller.
The action no longer deals with sending, and there's no display_name
param in this bit of code so it's a bit confusing.
@gravitystorm gravitystorm added the refactor Refactoring, or things that work but could be done better label Nov 2, 2022
@tomhughes
Copy link
Member

The only reason I had left those disabled instead of letting auto correct fix them was that it didn't manage to move the comments so needed a lot of manual intervention that I didn't have time for.

The flash one I've left disabled because it's very buggy and all of them are actually false positives...

@tomhughes tomhughes merged commit a8640d4 into openstreetmap:master Nov 2, 2022
@gravitystorm
Copy link
Collaborator Author

it didn't manage to move the comments so needed a lot of manual intervention that I didn't have time for.

Yep. Doing it manually also let me rearrange the non-restful ones too.

The flash one I've left disabled because it's very buggy and all of them are actually false positives...

Fixed upstream by rubocop/rubocop-rails#844 and so we just need to wait for the next rubocop-rails release.

@tomhughes
Copy link
Member

The flash one I've left disabled because it's very buggy and all of them are actually false positives...

Fixed upstream by rubocop/rubocop-rails#844 and so we just need to wait for the next rubocop-rails release.

That's one of them yes - there are about three or four that I was tracking as I wasn't entirely sure which one(s) we were hitting...

@gravitystorm gravitystorm deleted the action_order branch November 9, 2022 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring, or things that work but could be done better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants