[CLEANUP] Add DeclarationBlockParser class #1311
Conversation
|
I can only think that coverage has gone down because the code path in which However, it is probably impossible to pass a declaration block that would cause it to fail (and would depend on PHP environment configuration), because the regex is fairly simple. Maybe I should add a |
JakeQZ
force-pushed
the
cleanup/parse-declaration-block
branch
from
ed36927
to
ea7d2cd
Compare
JakeQZ
force-pushed
the
cleanup/parse-declaration-block
branch
2 times, most recently
from
db2db8b
to
b21eea3
Compare
JakeQZ
force-pushed
the
cleanup/parse-declaration-block
branch
8 times, most recently
from
8fd1198
to
88406d7
Compare
This has eliminated an unexercised statement. I also realized that the cache functionality wasn't being tested (I had previously thought to put in a test for that, then forgot). Now I am pretty sure all statements/constructs in the new class are covered by the test. But still coverage is down 0.02%. Note that there is a conditional I think the issue with the Coveralls test is that it is too simplistic. It just looks at the percentage of code units (statements?) that are covered. Here, I've replaced two completely-covered methods with one completely-covered method. Thus the amount of uncovered code, as a percentage, will inevitably increase. The metric that should be used for CI failure should be the actual amount of uncovered code, not the percentage of it as a whole. I would expect similar problems whenever any functionality is removed. Unless you can spot any peice of code in the new class that is not covered by tests, I suggest that the Coveralls CI failure be overridden in this case. And we should see if there is an alternative metric that can be used for CI failure (as suggested above), and if not create a ticket. Obviously review the code as usual :) |
|
Changing to draft again, as now dependent on #1313. Though the coverage metrics issue described above is still active - @oliverklee? |
Yes, I agree. I honestly don't know how the |
I've now checked the "leave PR comments" checkbox (which, for some reason) was not checked in the Coveralls configuration. Let's see. |
|
I've also removed access to the Coveralls Pro 3rd party app now. |
JakeQZ
force-pushed
the
cleanup/parse-declaration-block
branch
3 times, most recently
from
8a2abf0
to
8389ba4
Compare
|
This can be reviewed now. |
Seems to have worked. However, there was no comment made when I made the final repush to this PR branch. That may have been because the PR was still marked as draft at the time (and chaning PR status from 'draft' to 'ready for review' does not trigger the comment). |
This resolves three PHPStan errors. Instances of calls to `preg_split` in the two (identical) `parseCssDeclarationsBlock` methods are not updated - those are covered separately by #1311.
This resolves three PHPStan errors. Instances of calls to `preg_split` in the two (identical) `parseCssDeclarationsBlock` methods are not updated - those are covered separately by #1311.
Yes, that's what I think as well. (The CI for PRs only runs after a push.) |
oliverklee
force-pushed
the
cleanup/parse-declaration-block
branch
from
8389ba4
to
2b34d83
Compare
This is essentially a refactoring to move the `parseCssDeclarationsBlock` method that was duplicated in both `CssInliner` and `CssToAttributeConverter` to a separate class. As well as eliminating duplicate code/functionality, there are other benefits: - The method can be tested independently, which is duly done; - The results cache is now shared; - The functionality is readily available to other classes, e.g. for the solution to #1276 (as currently proposed). PHPStan errors from the original code have been resolved as follows: - Use `Preg::split` to wrap `preg_split` to handle unexpected errors; - Check the return value from `preg_match` precisely with `!== 1` instead of `!`. Longer-term, we should be able to use functionality from PHP-CSS-Parser for more accurate parsing (e.g. strings containing semicolons), whereupon this class can become a wrapper for that, but currently that functionality is not available in a standalone class.
oliverklee
force-pushed
the
cleanup/parse-declaration-block
branch
from
2b34d83
to
56ae6f0
Compare
This was added during development while `Preg::split` supported `PREG_SPLIT_OFFSET_CAPTURE` and thus could return an array of arrays, and the code contained a check for this. But before #1311 was completed, #1313 was implemented to avoid the headache of a dynamic return type. The check was removed, but not the corresponding comment explaining it.
#1321) This was added during development while `Preg::split` supported `PREG_SPLIT_OFFSET_CAPTURE` and thus could return an array of arrays, and the code contained a check for this. But before #1311 was completed, #1313 was implemented to avoid the headache of a dynamic return type. The check was removed, but not the corresponding comment explaining it.
This is essentially a refactoring to move the
parseCssDeclarationsBlockmethod that was duplicated in bothCssInlinerandCssToAttributeConverterto a separate class.As well as eliminating duplicate code/functionality, there are other benefits:
PHPStan errors from the original code have been resolved as follows:
Preg::splitto wrappreg_splitto handle unexpected errors;preg_matchprecisely with!== 1instead of!.Longer-term, we should be able to use functionality from PHP-CSS-Parser for more accurate parsing (e.g. strings containing semicolons), whereupon this class can become a wrapper for that, but currently that functionality is not available in a standalone class.