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

chore: switch to mago #501

Merged
merged 5 commits into from
Jan 10, 2025
Merged

chore: switch to mago #501

merged 5 commits into from
Jan 10, 2025

Conversation

azjezz
Copy link
Owner

@azjezz azjezz commented Dec 8, 2024

there is over 700 linting issues reported by mago, we will not be fixing those in this PR, but i will make a following PR to address them.

@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Status: In Progress This issue is being worked on, and has someone assigned. Type: Maintenance Updating phrasing or wording to make things clearer or removing ambiguity Type: Enhancement Most issues will probably ask for additions or changes. Subject: Dependencies Pull requests that update a dependency file labels Dec 8, 2024
@azjezz azjezz self-assigned this Dec 8, 2024
@azjezz azjezz requested a review from veewee December 8, 2024 20:26
@azjezz azjezz force-pushed the chore/switch-to-mago branch from 563246f to 0712fc0 Compare December 8, 2024 20:29
@azjezz azjezz marked this pull request as draft December 8, 2024 20:33
@azjezz azjezz force-pushed the chore/switch-to-mago branch from 84a0994 to a5404f1 Compare December 8, 2024 21:02
@azjezz azjezz marked this pull request as ready for review December 8, 2024 21:33
@azjezz
Copy link
Owner Author

azjezz commented Dec 8, 2024

@veewee the ci failure seems to be unrelated to this change, can you confirm?

I have also updated psalm so that might be the reason.

@veewee
Copy link
Collaborator

veewee commented Dec 9, 2024

I horizontally scrolled through the code-style changes and am OK with them.

That said, I don't know if all rules we applied in php-cs-fixer and phpcs are configured the same way as in mago.
On existing code, it's hard to tell everything is applied because it was already that way before.
Not sure if new code will also be changed according the specific rules we've set in the past. Did you base the formatter / linter rules on the existing once inside this repo or are there some of them missing? How can we make sure this will work as expected on new code as well?

the ci failure seems to be unrelated to this change, can you confirm?

The CI didn't fail before, so it must be the bump of the psalm dependency in here.
It's probably best to fix those issues regardless in here or to create a separate psalm bump issue.

@azjezz
Copy link
Owner Author

azjezz commented Dec 10, 2024

That said, I don't know if all rules we applied in php-cs-fixer and phpcs are configured the same way as in mago.

Yes, but it seems that php-cs-fixer and phpcs both were not able to actually apply some of those rules, for example, line soft limit rule in phpcs was never applied by phpcs, it only complained if a line exceeded that limit, while mago does split the line if is too long, and inlines it if it fits, an example can be seen in src/Psl/Async/sleep.php, where mago inlines the call because it fits into a single line, while both phpcs and php-cs-fixer ignore it.

since phpcs and php-cs-fixer are not able to split lines or inline them correctly, we previously had to set a super high line limit in phpcs to avoid having to fix the lines manually.

i personally think mago is doing a better job at applying psr-12 than both php-cs-fixer and phpcs.

@veewee
Copy link
Collaborator

veewee commented Dec 10, 2024

@azjezz

I'm sure mago is doing a good job regarding the rules it knows.

My remark was more about the rules that are not in PSR-12.

An non exhaustive list of examples:

  • sorting namespaces
  • adding strict_types declaration
  • using static:: in unit tests
  • Forcing final or abstract on all classes.
  • Forcing list syntax
  • ordered interfaces
  • static lambdas
  • ....

(It's basically the reason why we've choosen to use both php-cs-fixer and phpcs : to force multiple rules that the other tool does not know)

@azjezz
Copy link
Owner Author

azjezz commented Dec 10, 2024

well, that's the goal of dogfeeding :D to improve it by using it on a real world project.

as for the rule you listed.

  • sorting namespaces: not yet
  • adding strict_types declaration: yes, linter
  • using static:: in unit tests: not yet
  • Forcing final or abstract on all classes: neither phpcs or php-cs-fixer support this, but we can make it possible in mago.
  • Forcing list syntax: yes, linter
  • ordered interfaces: in what context?
  • static lambdas: not yet.

I'll make sure to add all the missing rules you mentioned in the next couple of days before merging this.

@veewee
Copy link
Collaborator

veewee commented Dec 10, 2024

as for the rule you listed.

* Forcing final or abstract on all classes: neither phpcs or php-cs-fixer support this, but we can make it possible in mago.

php-cs-fixer has https://cs.symfony.com/doc/rules/class_notation/final_class.html

* ordered interfaces: in what context?

php-cs-fixer uses https://cs.symfony.com/doc/rules/class_notation/ordered_interfaces.html

I'll make sure to add all the missing rules you mentioned in the next couple of days before merging this.

As mentioned, it is a non exhaustive list. If we want to fully remove both phpcs and php-cs-fixer, we should cover all the rules that we currently apply in both of those tools or agree on not using specific rules anymore.

@azjezz
Copy link
Owner Author

azjezz commented Dec 10, 2024

php-cs-fixer has https://cs.symfony.com/doc/rules/class_notation/final_class.html

we had this disabled because we couldn't make it work in Psl sadly.

php-cs-fixer uses https://cs.symfony.com/doc/rules/class_notation/ordered_interfaces.html

this is easy to add, but not sure if we want to.


As mentioned, it is a non exhaustive list. If we want to fully remove both phpcs and php-cs-fixer, we should cover all the rules that we currently apply in both of those tools or agree on not using specific rules anymore.

I don't see the need for us to do that, as long as the code is psr-12 compatiable, which is what we had set those two tools to do.

other rules can be implemented one-by-one, and that is the goal of having mago here, to act as an example+test case.

Signed-off-by: azjezz <azjezz@protonmail.com>
Signed-off-by: azjezz <azjezz@protonmail.com>
Signed-off-by: azjezz <azjezz@protonmail.com>
Signed-off-by: azjezz <azjezz@protonmail.com>
@azjezz azjezz force-pushed the chore/switch-to-mago branch from 9f3da59 to 70b9b86 Compare December 29, 2024 21:30
Signed-off-by: azjezz <azjezz@protonmail.com>
@azjezz azjezz merged commit c0c4bb6 into next Jan 10, 2025
34 checks passed
@azjezz azjezz deleted the chore/switch-to-mago branch January 10, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: In Progress This issue is being worked on, and has someone assigned. Subject: Dependencies Pull requests that update a dependency file Type: Enhancement Most issues will probably ask for additions or changes. Type: Maintenance Updating phrasing or wording to make things clearer or removing ambiguity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants