search

moodlehq / moodle-local_codechecker

A Moodle local plugin providing a simple web UI to run the MoodleCS coding style checks with PHP_CodeSniffer.
63 stars 72 forks source link

Incorrect checks applied to CSV files #203

Closed timhunt closed 2 years ago

timhunt commented 2 years ago

https://docs.fileformat.com/spreadsheet/csv/ says "Each record is located on a separate line, delimited by a line break (CRLF)."

CodeChecker is telling me

"Windows (CRLF) line ending instead of just LF (reporting only first occurrence)"

on my CSV file. I think CodeChecker is wrong.

timhunt commented 2 years ago

Seems to only appy when running CodeChcker form the Moodle UI, not in github actions.

stronk7 commented 2 years ago

I bet that is one of those "special" (apart from PHP_Codesniffer) checks that were added to the plugin. Lately have found myself fixing some of them. Really they don't add much, but confusion, heh.

Will take a look to this case soon...

stronk7 commented 2 years ago

Yeah, confirmed. That check is there since the night of times:

https://github.com/moodlehq/moodle-local_codechecker/commit/1d808e0e#diff-3494b45596f0f648d894a5f22070eaae01f3e0ea664be0430b8d23eb7d93e35aR220-R224

And I think it just means that no file within Moodle should be CRLF ended ever. Including CSV files.

Looking to load_csv_content(), we always normalise everything (CR, LF and CRLF) to LF, and then use fgetcsv() with those, already converted, LF.

So I think it's ok to require all CSV files in codebase (some fixture, I bet?) to be LF and not CRLF. Hence, maybe this should be closed?

timhunt commented 2 years ago

I think that the point of testing is to verify that your system will work for users.

Users will create CSV files by saving from Excel, so we should check that those import correctly. Therefore, test fixture CVS files are better tests if they have CR LF. Therefore we should allow it.

At least, that is what I think.

stronk7 commented 2 years ago

Would be ok enough to make an exception for anything under a "tests/fixtures" dir and allow everything there?

Allow everything = I mean, don't verify EOLs in CSV files under "tetst/fixtures" dirs.

Ciao :-)

timhunt commented 2 years ago

It might be.

Or, that might just be a confusing inconsistency. My feeling is the same rules should apply to a file type everywhere.

stronk7 commented 2 years ago

Yeah, the rule is / has been pretty consistent since the beginning, LOL. Don't allow CRLF files ever, point!.

It's not me the one proposing to add exceptions, just wanted to restrict them to the real cases where we can effectively accept CRLF CSV files. And the only case is fixtures.

If for some (strange) reason we want a CSV file elsewhere, it should be a correct LF one (for example: admin/tool/uploaduser/example.csv).

timhunt commented 2 years ago

We don't allow CRLF in .php, .css, .txt,.. Obviously that is right.

But, CRLF is in the definition of .csv. (https://www.rfc-editor.org/rfc/rfc4180.html). I know .csv is not massively standardised, but requiring all our example .csv files to break the standard seems a strange decision.

stronk7 commented 2 years ago

I've created #205 that enabled the use of CRLF CSV files under the fixtures directory.

I really don't see much benefit allowing them everywhere. The reality (the facto "standard") is that both work the same and we aren't going to add CRLF examples elsewhere (haven't in 20 years).

I understand that we should be able to test that CRLF imports work and that's exactly what the PR provides.

So, in practice, with that PR, you will be able to use those CRLF CSV fixtures and done. I think nobody is going to convince the other, LOL.

Ciao :-)

timhunt commented 2 years ago

Fair enoguh. Thanks for doing it Eloy.