Skip to content

Typehint some Controllers#5106

Merged
Alkarex merged 17 commits intoFreshRSS:edgefrom
ColonelMoutarde:feature/add-typehinting-to-controllers
Mar 21, 2023
Merged

Typehint some Controllers#5106
Alkarex merged 17 commits intoFreshRSS:edgefrom
ColonelMoutarde:feature/add-typehinting-to-controllers

Conversation

@ColonelMoutarde
Copy link
Copy Markdown
Contributor

@ColonelMoutarde ColonelMoutarde commented Feb 13, 2023

Changes proposed in this pull request:

  • typehinting
  • phpstan level 8

Pull request checklist:

  • clear commit messages
  • code manually tested

@Alkarex Alkarex modified the milestones: 1.21.0, 1.22.0 Feb 22, 2023
@Alkarex Alkarex added the System care Everything related to system care label Feb 22, 2023
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Mar 16, 2023

Good start, but this not level 9 (517 errors), and not even level 6 (87 errors).

Example:

$ vendor/bin/phpstan analyse --level 6 app/Controllers/
...
 ------ ------------------------------------------------------------------------------ 
  Line   tagController.php                                                             
 ------ ------------------------------------------------------------------------------ 
  :108   Method FreshRSS_tag_Controller::renameAction() has no return type specified.  
 ------ ------------------------------------------------------------------------------ 
...
[ERROR] Found 87 errors

Could you try to pass at least level 6 for the files you are addressing?

I am wondering how did you test that this passed level 9? Maybe a wrong command?

@ColonelMoutarde
Copy link
Copy Markdown
Contributor Author

Good start, but this not level 9 (517 errors), and not even level 6 (87 errors).

Example:

$ vendor/bin/phpstan analyse --level 6 app/Controllers/
...
 ------ ------------------------------------------------------------------------------ 
  Line   tagController.php                                                             
 ------ ------------------------------------------------------------------------------ 
  :108   Method FreshRSS_tag_Controller::renameAction() has no return type specified.  
 ------ ------------------------------------------------------------------------------ 
...
[ERROR] Found 87 errors

Could you try to pass at least level 6 for the files you are addressing?

I am wondering how did you test that this passed level 9? Maybe a wrong command?

Hi,
the files that you mention as being in error, are files that I have not touched. I have not done all the files in the controller folder

@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Mar 18, 2023

Check again: the example I gave, tagController.php, is one you have touched, and it does not pass level 6 ;-)

image

Fair enough if you have not addressed all the controllers, but in this case, the PR description is inaccurate and needs to be corrected...

@Alkarex Alkarex mentioned this pull request Mar 18, 2023
2 tasks
@ColonelMoutarde ColonelMoutarde changed the title Typehint to Controllers Typehint some Controllers Mar 20, 2023
@ColonelMoutarde
Copy link
Copy Markdown
Contributor Author

Up to level 8

@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Mar 21, 2023

Ok, looks good now. The command to check is something like:

$ vendor/bin/phpstan analyse --level 6 app/Controllers/apiController.php app/Controllers/authController.php app/Controllers/categoryController.php app/Controllers/configureController.php app/Controllers/entryController.php app/Controllers/errorController.php app/Controllers/importExportController.php app/Controllers/javascriptController.php app/Controllers/statsController.php app/Controllers/subscriptionController.php app/Controllers/tagController.php 
Note: Using configuration file /home/alexandre/GitHub/FreshRSS/phpstan.neon.
 11/11 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors

@Alkarex Alkarex merged commit 247215f into FreshRSS:edge Mar 21, 2023
@ColonelMoutarde ColonelMoutarde deleted the feature/add-typehinting-to-controllers branch May 3, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

System care Everything related to system care

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants