Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add same github actions than pylint but withtout benchmark/spelling #947

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Apr 17, 2021

Description

Introduce github action because appveyor is hell and travis is changing his commercial proposal. Also python 3.10 seems to work for astroid so we might as well test it.

Type of Changes

Type
🔨 Refactoring

Related Issue

@Pierre-Sassoulas Pierre-Sassoulas changed the title Add same github actions than pylint but withtout benchmark Add same github actions than pylint but withtout benchmark/spelling Apr 17, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.5.4 milestone Apr 17, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting the work here! I never really got to it.

Some general comments:

  • Should we add the classifier for py310 if the tests already pass?
  • Travis does run pylint as well. This isn't covered here
  • The requirement files probably need some work. From looking at the tox.ini file, we probably need to add a requirements_pypy.txt and a requirements_six.txt file as well. https://github.com/PyCQA/astroid/blob/f2b197a4f8af0ceeddf435747a5c937c8632872a/tox.ini#L13-L28
  • All tests (linux and window) should be run with and without six. Maybe this could work with strategy.matrix?

.travis.yml Show resolved Hide resolved
requirements_test.txt Outdated Show resolved Hide resolved
requirements_test_pre_commit.txt Outdated Show resolved Hide resolved
requirements_test_min.txt Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Apr 18, 2021

I left the x-six and pylint jobs in the travis.yml. Also if the tests are passing without numpy it means we don't need numpy during tests so I removed it in order to be able to have python 3.10.

@cdce8p
Copy link
Member

cdce8p commented Apr 23, 2021

I opened #964 and #965 which should simplify this MR, once they are merged.
Can you rebase it as well? It still includes the backports.functools_lru_cache dependency and the issue with requirements_pre_commit.txt which are already fixed.

After those things are done, I believe we only need to check that all the right requirements are installed and the cache key is generated correctly and it should be good to go. Maybe we should also pin the requirements in requirements_test_brain.txt?

If you like, let me know once you're done with the rebase and I can take another look regarding the dependencies.

@Pierre-Sassoulas
Copy link
Member Author

#965 is not mergeable right now, I tried to do it too but we need to think about why six or not six is required for tests (I don't know much about six). So I think this is reviewable now.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the add-github-actions branch 2 times, most recently from cc0d8d1 to eed67ba Compare April 23, 2021 19:24
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the add-github-actions branch 2 times, most recently from df4cff4 to a6b1031 Compare April 23, 2021 19:45
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few small things, mainly requirements_test_brain.txt. That file isn't loaded inside requirements_test.txt, which is the right decision, so it needs to be installed manually.

appveyor.yml Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
python -m venv venv
. venv/bin/activate
python -m pip install -U pip setuptools wheel
pip install -U -r requirements_test.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pip install -U -r requirements_test.txt
pip install -U -r requirements_test.txt -r requirements_test_brain.txt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason why you didn't add this one here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No my bad !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you what do fix it or shall I? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix in a MR for fixing pylint's warning again. (pylint's master changed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably pin the pylint version for the formatting checks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought that as well, we don't want to have to fix unrelated pylint warning in a MR just because pylint changed.

See : https://github.com/PyCQA/astroid/pull/968/files

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas merged commit 682ec6d into master Apr 23, 2021
@Pierre-Sassoulas Pierre-Sassoulas deleted the add-github-actions branch April 23, 2021 21:06
- python: pypy3
env: TOXENV=pypy
- python: 3.6
env: TOXENV=py36,py36-six
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed, we probably broke the deploy stage by removing py36 here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was already broken but not the same way 😄 If you want to fix it feel free.. I'm aiming to fix pylint's warning and finish #937 before releasing 2.5.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants