search

apecs-org / Polar-EO-Database

Polar Earth Observation Database of satellite sensors
GNU General Public License v3.0
21 stars 5 forks source link

Add super-linter #3

Closed weiji14 closed 2 years ago

weiji14 commented 2 years ago

Hi @AdrienWehrle, great work on setting this up! I had a quick look through the GitHub Actions workflow, and thought it might be better to separate the JSON linting from the actual Google Sheets syncing. I.e. run the sync only on pushes to the 'main' branch, rather than on every pull request (in case anyone opens a random pull request that overwrites the Google Sheets).

So this pull request adds a separate linter workflow using https://github.com/github/super-linter. I've not used it before, but apparently it lints just about anything (JSON, Python files, etc), so I thought to give it a try. The linter should also report which file and/or which line number the problem is at, so we can troubleshoot directly.

weiji14 commented 2 years ago

I'm getting this error with the JSON linter at https://github.com/APECS-Earth-Observation/Polar-EO-Database/runs/5644882428?check_suite_focus=true#step:4:90

2022-03-22 13:35:56 [ERROR]   Found errors in [eslint] linter!
2022-03-22 13:35:56 [ERROR]   Error code: 1. Command output:
------

/github/workspace/data/AATSR.json
  4:33  error  Parsing error: Unexpected identifier 'NaN'

Seems like it doesn't like the NaN values. Should we be using 'null' instead according to https://stackoverflow.com/questions/21120999/representing-null-in-json?

AdrienWehrle commented 2 years ago

Hi @AdrienWehrle, great work on setting this up! I had a quick look through the GitHub Actions workflow, and thought it might be better to separate the JSON linting from the actual Google Sheets syncing. I.e. run the sync only on pushes to the 'main' branch, rather than on every pull request (in case anyone a random pull request overwrites the Google Sheets).

So this pull request adds a separate linter workflow using https://github.com/github/super-linter. I've not used it before, but apparently it lints just about anything (JSON, Python files, etc), so I thought to give it a try. The linter should also report which file and/or which line number the problem is at, so we can troubleshoot directly.

Hi @weiji14 ! That completely makes sense and was in my TODO! Super nice that you worked on it!

AdrienWehrle commented 2 years ago

I'm getting this error with the JSON linter at https://github.com/APECS-Earth-Observation/Polar-EO-Database/runs/5644882428?check_suite_focus=true#step:4:90

2022-03-22 13:35:56 [ERROR]   Found errors in [eslint] linter!
2022-03-22 13:35:56 [ERROR]   Error code: 1. Command output:
------

/github/workspace/data/AATSR.json
  4:33  error  Parsing error: Unexpected identifier 'NaN'

Seems like it doesn't like the NaN values. Should we be using 'null' instead according to https://stackoverflow.com/questions/21120999/representing-null-in-json?

Yes indeed, NaN is not supported in JSON (which is probably a good thing) and we should remove all nans from the current JSON entries. But those JSON files are anyway simply examples to get started with at the moment. If you look carefully, you'll see some entries were meant for Google sheets and are not properly converted to JSON so we will have to change that at some point anyway.

But for the time being, I'll write a short script that deletes all entries with NaN values and add it to this PR, then all tests should pass!

AdrienWehrle commented 2 years ago

I think we should also run the tests on the database (see below) on PR so that potential contributors see directly if their contribution meet the requirements or not! Do you agree? This would mean, for clarity, probably creating a new YAML file with tests on PR.

At the moment there is hardly anything in the tests, but I'm working on preliminary field tests at the moment and will push soon!

weiji14 commented 2 years ago

I think we should also run the tests on the database (see below) on PR so that potential contributors see directly if their contribution meet the requirements or not! Do you agree? This would mean, for clarity, probably creating a new YAML file with tests on PR.

If I understand this correctly, you mean setting up a GitHub Actions workflow (a YAML file) that checks a new contributor's edits to the database (which are the JSON files in data/)?

At the moment there is hardly anything in the tests, but I'm working on preliminary field tests at the moment and will push soon!

Ok, feel free to push directly to this branch, or open another branch/Pull Request and I can help review it :smiley:

AdrienWehrle commented 2 years ago

I think we should also run the tests on the database (see below) on PR so that potential contributors see directly if their contribution meet the requirements or not! Do you agree? This would mean, for clarity, probably creating a new YAML file with tests on PR.

If I understand this correctly, you mean setting up a GitHub Actions workflow (a YAML file) that checks a new contributor's edits to the database (which are the JSON files in data/)?

Exactly, I started some work here (linked to #4) few minutes ago and will do more shortly!

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging 55b4965d77d7fbc63f37fca6b7342a39f8fe77a0 into f41c0d84d6467fa07651461995e426d0e0017d4a - view on LGTM.com

new alerts:

AdrienWehrle commented 2 years ago

Will fix my issue tonight! :)

lgtm-com[bot] commented 2 years ago

This pull request introduces 2 alerts when merging 52a1c771d76f248e325f237518e6b9668dc1d70b into f41c0d84d6467fa07651461995e426d0e0017d4a - view on LGTM.com

new alerts:

AdrienWehrle commented 2 years ago

Exactly, I started some work here (linked to https://github.com/APECS-Earth-Observation/Polar-EO-Database/issues/4) few minutes ago and will do more shortly!

I will add the testing on PR in the associated PR, let's just merge this one first :)