DataTables / Editor-PHP

PHP server-side libraries for Editor
Other
35 stars 22 forks source link

Fix spacing according PSR-12 #27

Closed mvorisek closed 1 year ago

mvorisek commented 1 year ago

extracted from #8

pure non-functional change, code AST is unchanged

CS is asserted using CI

AllanJard commented 1 year ago

I'm afraid I'll not pull in a change to spaces rather than tabs. I know PSR-12 says 4 spaces, but I massively prefer the flexibility of tabs.

mvorisek commented 1 year ago

In PHP world, tabs are very rare and developers hate them. But I respect your decision and I changed the indentation to tabs.

AllanJard commented 1 year ago

Thanks! Much appreciated - and in general for what you are doing here! Amazing work. You need to let me know your DataTables.net account name so I can get that license set up for you.

AllanJard commented 1 year ago

A follow up to this - is it possible to exclude a file from the linting? The Htmlawed.php file specifically is from a third party and I think it would be correct to just use it in the format they distribute it.

mvorisek commented 1 year ago

It is possible, but I prefer to keep it fixed/linted when distributed thru the same repo, as on an update, the linter can be easily run and the file autofixed (and diffed mefore commit). Or include the dep using composer, but then then install would require composer.

AllanJard commented 1 year ago

Composer isn't an option unfortunately. I feel that since it is a third party library I would prefer to keep it in its original format.

I've also just made commit c943b11db56e55975d5882ef76cffc2b9934c859 as phpDocumentor doesn't like the ...$variableName syntax. Apparently it uses $variableName,.... That is causing the lint tests to fail. I'm not clear on how to put a bypass for that.

mvorisek commented 1 year ago

It is your job to keep the CI green :)

About the variadic phpdoc - the format I used is correct, phpstan grammar: https://github.com/phpstan/phpdoc-parser/blob/1.22.1/doc/grammars/type.abnf#L44C41-L44C68

AllanJard commented 1 year ago

Yup - bug filed. We'll see what they say. At the moment I need the phpDocumentor output, so I'll continue using that syntax until it can be addressed or there is a better option for the auto doc generation.

Thanks again.

AllanJard commented 1 year ago

Sorry to keep asking questions - I'm trying to get a better handle on things! I've got phpstan running locally and have fixed a few and updated the baseline as a result. However, the github tests run are failing on errors that are in the baseline. My understanding of the baseline is that they would effectively be ignored. So I'm not sure why it is packing up these errors (and only these errors). Any ideas?

mvorisek commented 1 year ago

Phpstan baseline is list of project's issues that are to be temporary ignored. Ideally there should be no baseline. To fix the phpstan CI once some issues is fixed, you can a) regenerate the baseline using phpstan, OR b) delete the no-longer needed entries from the baseline manually.

AllanJard commented 1 year ago

Right that makes sense (and removal of the baseline is something I've started working towards).

What I'm confused about is why the Github CI is showing errors with the current baseline, when, with the same baseline locally it is showing no errors (since they are all ignored atm).

I'd have thought they would be the same.

mvorisek commented 1 year ago

There might be minor differences due different php versions - run phpstan using the same version as the CI.

I also recomment to lower the phpstan level to 3 - 4, regen the baseline, fix the errors, and then upgrade the phpstan level back. But do not commit the lowered phpstan config of course.