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

Allow configuration with EloquentOrderByToLatestOrOldestRector rule #147

Closed
johnbacon opened this issue Oct 12, 2023 · 5 comments · Fixed by #148
Closed

Allow configuration with EloquentOrderByToLatestOrOldestRector rule #147

johnbacon opened this issue Oct 12, 2023 · 5 comments · Fixed by #148

Comments

@johnbacon
Copy link
Contributor

By default, EloquentOrderByToLatestOrOldestRector modifies all orderBy and orderByDesc methods to use latest() or oldest() instead.

Our team feels we'd like to do this only regarding timestamps, as oldest('name') or similar could be confusing.

As such, some configuration allowing for explicit column names you'd like to change to oldest() or latest() is desired.


This suggestion was also briefly mentioned in the relevant PR:

These changes apply to all columns in our project, not just date/datetime columns. Is that intended behavior? If so, I'm wondering if rector-laravel is open to configuration, so we can specify which columns should and shouldn't have these rules applied.

@peterfox
Copy link
Collaborator

@johnbacon what's your thoughts, a list of columns or simply a regex to match? At current the rule will refactor if a variable is used. If this was enabled. It would only refactor with string literals.

@johnbacon
Copy link
Contributor Author

johnbacon commented Oct 13, 2023

@peterfox The implementation allows for wildcards and a list of columns, but do you feel regex would provide more benefit on top of that?

The fact that static analysis can't determine the name of a column within a variable was a bummer to learn. I "got around" that by allowing you to specify the name of a variable, e.g. '$columnName'. But if you always want to refactor all variables in an orderBy(), I'm not sure what would be best. Maybe an additional option you config? Maybe defining '$*' or similar would work?

@peterfox
Copy link
Collaborator

Examining variable names is a bit risky. I'd likely stick to string literals. Regex, to me, makes the most sense rather than listing all possibilities someone might have. Wildcard could work, but sometimes it's more effort when Regex is more flexible, and the example of ends with _at would be easy enough.

@johnbacon
Copy link
Contributor Author

Is that something you could possibly lend a hand on? I'm less comfortable with regex and even more so in the Laravel/Rector ecosystem, unfortunately.

@peterfox
Copy link
Collaborator

@johnbacon Sure, I'll see if I can find time to work on it.

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 a pull request may close this issue.

2 participants