Switch from CircleCI to GitHub Actions

[HAEKADI]

Related to openfisca/openfisca-core/#1030 Related to openfisca/openfisca-core/#1027 Related to openfisca/openfisca-france/#1663 Closes #114

GitHub Actions

• Technical improvement.
• Impacted areas: CI configuration
• Details:
  • Switch from CircleCI to GitHub Actions

[HAEKADI]

@MattiSG @maukoquiroga check-style in the Makefile uses both pylint and flake8 in Country-template but only flake8 in OpenFisca France.

The old CI did not include a linting job #114, when adding it to the new CI pylint causes the following error:

Is there a consensus on whether one or both should be used?

[MattiSG]

Unless they do different things, I am in favour of having only one linter, to make linting faster and more deterministic. I see that on Core, there's only flake8.

@HAEKADI didn't you mention superlinter? Do you think it would be appropriate here?

[HAEKADI]

@MattiSG Yes, I have. It would definitely be appropriate. You can find the tests I ran with it here.

But if my memory serves me well, we decided to add it in a separate PR after the switch to GitHub Actions as to not introduce too many changes at once.

[MattiSG]

we decided to add it in a separate PR after the switch to GitHub Actions as to not introduce too many changes at once

We did indeed 🙂 Now, if we have to reconsider the current way of linting, I just wanted to throw this in 😉

[bonjourmauko]

Hi !

Is there a consensus on whether one or both should be used?

We have agreed on the rules to encforce, that's why you will find them in the country and extension templates, yet still not implemented them in openfisca/openfisca-core#960 .

Unless they do different things, I am in favour of having only one linter, to make linting faster and more deterministic. I see that on Core, there's only flake8.

flake8 and pylint are lint "engines", they implement style codes, like PEP8 and so on.

What they do differently is hang indents (E128/133), a very specific style of the older OpenFisca community.

Other than that, I prefer whatever comes closes to the rules we have agreed upon (that excludes black for example).

Specific example: in OpenFisca, a Variable is a class, but it is snake cased, so the only prerequisite of a linter for me is that it can be highly configurable.

@HAEKADI didn't you mention superlinter? Do you think it would be appropriate here?

Haven't heard of that one, I'll take a look.

The minimal set of style rules for me is the one in the setup.cfg, well the exceptions:

; D101:                   Variables already provide label/description
; D107:                   We do not document __init__ method
; D401:                   We do not require the imperative mood
; E128/133:               We prefer hang-closing visual indents
; E251:                   We prefer `function(x = 1)` over `function(x=1)`
; E501:                   We do not enforce a maximum line length
; W503/4:                 We break lines before binary operators (Knuth's style)

; C0103:                  We (still) snake case variables and reforms
; C0115:                  Variables already provide label/description
; C0301:                  We do not enforce a maximum line length
; E0213:                  This requires changes in OpenFisca-Core
; E1101:                  False positive, as entities have members
; E1102:                  False positive, as entities are callable
; W0621:                  We name variable values the variable name
; W1203:                  We prefer to log with f-strings

Beyond that, I'm OK with whatever is faster and more deterministic as @MattiSG said.

Update

superlinter provides pylint, flake8, and isort, so I think the current ruleset is enforceable with it! 🙌

[HAEKADI]

Thank you for the clarification @maukoquiroga! I started implementing super-linter in Openfisca France here. I used super-linter not only for new scripts but on the existing code-base and there were too many errors to count. I was not aware that black was excluded and consequently started correcting those errors 😬

[bonjourmauko]

I don't have a clear view on latest discussions in the French community, but for the historical record black has been rejected twice while I was leading product. I was agnostic since, now I'm more on the reluctant side.

used super-linter not only for new scripts but on the existing code-base and there were too many errors to count.

I can relate to that, see https://github.com/openfisca/openfisca-core/pull/960 and https://github.com/openfisca/openfisca-france/issues?q=style+author%3Amaukoquiroga.

[HAEKADI]

I see. Maybe it's time to put pen to paper and have a clear definition of linting rules across all repos. I see you already started working on it @maukoquiroga with openfisca/openfisca-core#960.

[bonjourmauko]

I see. Maybe it's time to put pen to paper and have a clear definition of linting rules across all repos. I see you already started working on it @maukoquiroga with openfisca/openfisca-core#960.

That would be great!!!

See one of the discussions on black for example: https://github.com/openfisca/openfisca-core/pull/977

I've started https://github.com/openfisca/openfisca-core/blob/master/STYLEGUIDE.md and I'm happy to help if you decide to continuing efforts on style coherence —which I think is a great idea.

It is an idea that has been going on and proposed by some members. See https://github.com/openfisca/country-template/pull/111/files for example, beyond the actual implementation (pre-commit), it would be great to have all linting rules in a separated repo and pull them as a development dependency.

[HAEKADI]

I see. There are certainly many ways to go about doing that. As a starting point, I suggest implementing these rules, in the new CI, with super-linter, across all repos (France, Country and Core) as discussed before, to have a unified style.

[MattiSG]

If I understand correctly, super-linter is just a way to ease working with linters on GitHub Actions:

The design of the Super-Linter is currently to allow linting to occur in GitHub Actions as a part of continuous integration occurring on pull requests as the commits get pushed. It works best when commits are being pushed early and often to a branch with an open or draft pull request. There is some desire to move this closer to local development for faster feedback on linting errors but this is not yet supported.

However, in https://github.com/openfisca/openfisca-core/issues/1040, we decided to focus on consolidating tasks in a task manager, not in CI. Thus, I understand that if we want to call both flake8 and pylint, this should be wrapped in a task that is also executable locally and whose dependencies are declared and installed explicitly, not only indirectly through super-linter.

I also see in https://github.com/openfisca/country-template/pull/115/commits/aaea4ddd6e0566609b5a1265d40f07a6da6fc4f4 that the so-called “linting scripts” were removed, but these scripts do more than just activate linting: most of all, they select the subset of files to be linted in order to accelerate the process. I am thus not sure this should just be removed.

[MattiSG]

As discussed this morning with @HAEKADI, based on the conclusions from https://github.com/openfisca/openfisca-core/issues/1040, let's focus this PR on being isofunctional and moving to GitHub Actions without changing the services.

[HAEKADI]

I removed super-linter. I just have to check deploy. @MattiSG could you please add the pypi password to GitHub secrets?

[MattiSG]

could you please add the pypi password to GitHub secrets?

Done!

[HAEKADI]

The deploy seems functional.

[HAEKADI]

@MattiSG I tested pip install --editable ."[dev]"many times and it works fine for me 🤔

[MattiSG]

I deactivated required status checks to enable merging this PR, please ping an admin once merged to make sure the GitHub Actions status checks are made mandatory 🙂