Travis: Enforce phpcs line length + whitespace#3488
Travis: Enforce phpcs line length + whitespace#3488Alkarex merged 10 commits intoFreshRSS:masterfrom
Conversation
Yes 🎉 |
aledeg
left a comment
aledeg
left a comment
There was a problem hiding this comment.
I am surprised that there is not more impacted files :)
| $sql .= ' AND `lastSeen` < (SELECT `lastSeen`' | ||
| . ' FROM (SELECT e2.`lastSeen` FROM `_entry` e2 WHERE e2.id_feed = :id_feed2' | ||
| . ' ORDER BY e2.`lastSeen` DESC LIMIT 1 OFFSET :keep_min) last_seen2)'; | ||
| . ' FROM (SELECT e2.`lastSeen` FROM `_entry` e2 WHERE e2.id_feed = :id_feed2' | ||
| . ' ORDER BY e2.`lastSeen` DESC LIMIT 1 OFFSET :keep_min) last_seen2)'; |
There was a problem hiding this comment.
Here you could use the NOWDOC notation.
There was a problem hiding this comment.
Could be, but it is only a part of a longer string construction
| protected function sqlListEntriesWhere($alias = '', $filters = null, $state = FreshRSS_Entry::STATE_ALL, | ||
| $order = 'DESC', $firstId = '', $date_min = 0) { |
There was a problem hiding this comment.
In case like that, don't you think that having one argument per line is more readable?
protected function sqlListEntriesWhere(
$alias = '',
$filters = null,
$state = FreshRSS_Entry::STATE_ALL,
$order = 'DESC',
$firstId = '',
$date_min = 0
) {There was a problem hiding this comment.
Potentially yes for here, although I am not so found of excessive lines in general, as it generates more vertical scrolling, which reduces my ability to get the bigger picture of a piece of code.
In any case, the ambition of this PR (which is already large :-)) was just to fix the biggest problems - there is still room for improvement
| FreshRSS_UserDAO::touch(); | ||
| if (is_array($ids)) { //Many IDs at once (used by API) | ||
| if (true) { //Speed heuristics //TODO: Not implemented yet for SQLite (so always call IDs one by one) | ||
| //if (true) { //Speed heuristics //TODO: Not implemented yet for SQLite (so always call IDs one by one) |
There was a problem hiding this comment.
I'd rather remove it completely than having some commented code.
There was a problem hiding this comment.
Yes indeed, but in this specific case, it is to match the construct of the parent code and make the TODO easier to understand & address:
FreshRSS/app/Models/EntryDAO.php
Line 338 in 064288e
|
I realised that we can include |
| .PHONY: lint | ||
| lint: bin/phpcs ## Run the linter on the PHP files | ||
| $(PHP) ./bin/phpcs . --standard=phpcs.xml --warning-severity=0 --extensions=php -p | ||
| $(PHP) ./bin/phpcs . -p -s |
There was a problem hiding this comment.
@aledeg Does that look good to you for the Makefile?
There was a problem hiding this comment.
Now file extensions are defined in phpcs.xml
-s is to include the name of broken rules in the outputs
Regression from FreshRSS#3488
3 participants
Give up on allowing a mixture of tabs and spaces, in order to be able to easily enforce line length and whitespace rules.
See [CI] Add PHP 7.3 to Travis configuration #2317 (comment)
Report warnings in Travis, and not only errors
Lines longer than lineLimit=165 are reported as a warning (this is the size of a GitHub window)
Lines longer than absoluteLineLimit=190 are now an error (that works also for our .phtml template files)
Fixed the lines that did not comply with those rules. Mainly some lines that were much too long, but also a some wrong whitespace
Now also include our tests
Incidently, works also for our .css files
Now we can run:
phpcs -p --extensions=phtml,php,css .My motivation came from giving a try to https://marketplace.visualstudio.com/items?itemName=ikappas.phpcs that provides instant feedback in the editor.
Before this patch, there were warnings all over the place.