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

Improved code quality #188

Open
wants to merge 23 commits into
base: 6.0
Choose a base branch
from
Open

Improved code quality #188

wants to merge 23 commits into from

Conversation

FelixJacobi
Copy link
Member

@FelixJacobi FelixJacobi commented Mar 15, 2024

@SamuelWei Interested in your thoughts ;) .

fix #177
fix #162

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (40494d0) to head (a623baf).

Additional details and impacted files
@@             Coverage Diff              @@
##                6.0     #188      +/-   ##
============================================
- Coverage     99.13%   99.11%   -0.03%     
+ Complexity      440      439       -1     
============================================
  Files            52       52              
  Lines          1037     1013      -24     
============================================
- Hits           1028     1004      -24     
  Misses            9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FelixJacobi
Copy link
Member Author

FelixJacobi commented Mar 15, 2024

  • We must fine tune rector, to not add readonly on wrong properties.
  • Review each change very carefully, Tests seems to be running for now.
  • Rector cannot modernize 100%, manual short check if anything planned for 6.0 is really done.

@FelixJacobi FelixJacobi marked this pull request as ready for review March 15, 2024 02:54
@SamuelWei
Copy link

I think we should move all enums to the same folder/namespace.

However this is a BC. I suggest

  1. Copy the current enum classes to the enum folder and refactor using the logic used in 5.x
  2. Replacing the consts in the current namespace using aliases and deprecate all
  3. Refactor to native enums in 6.0 and remove old classes

@FelixJacobi
Copy link
Member Author

I think we should move all enums to the same folder/namespace.

However this is a BC. I suggest

1. Copy the current enum classes to the enum folder and refactor using the logic used in 5.x

2. Replacing the consts in the current namespace using aliases and deprecate all

3. Refactor to native enums in 6.0 and remove old classes

I think, since adding types and enums to the whole code base is already a change with big impact, I would schedule code restructuring (e.g. where to place classes) for 7.0.

From consumer perspective, the good thing for now, that the baked enums on using works like the constants and mabe classes before, you don't need to change your code when upgrading. And remember: 6.0 cannot trigger new deprecations ;) (ok, except for the one in the BigBlueButton constructor, which was needed to catch typing issues when the class is initialized without environment).

@FelixJacobi
Copy link
Member Author

FelixJacobi commented Mar 15, 2024

To add enums here is practical, I think, we cannot introduce and use them in 5.x, since that one must still support PHP 7.4. In 5.x and 6.1, it also would mean being a breaking change.

In theory, it could introduce a few breaking changes, therefor it fits perfectly into a major release.

@SamuelWei
Copy link

To add enums here is practical, I think, we cannot introduce and use them in 5.x, since that one must still support PHP 7.4. In 5.x and 6.1, it also would mean being a breaking change.

In theory, it could introduce a few breaking changes, therefor it fits perfectly into a major release.

I opened #197 to show my proposed solution to move all enums to a new namespace first, allowing us to convert them to native enums later

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 this pull request may close these issues.

None yet

2 participants