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

Adding flake8, pytest, tox, and github actions #59

Merged
merged 14 commits into from
Dec 18, 2023

Conversation

AndKaminer
Copy link
Contributor

As the title says.

Never worked with any of these technologies, so this is an amalgamation of various tutorials. It works fine on my branch, but I wouldn't necessarily take me at my word here.

Currently we fail flake8 pretty badly right now, but we can deal with that in another PR. This finishes up the list of objectives we had in #54. I feel content closing the issue once we actually start passing flake8, which I can get done when it isn't 1:30 am.

Copy link
Contributor

@aphedges aphedges left a comment

Choose a reason for hiding this comment

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

I have no clue how to do it with tox, but the pre-commit checks (declared in .pre-commit-config.yaml) should also be run in CI.

Comment on lines 17 to 19
- uses: actions/checkout@v2
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

You should upgrade to the latest version of these actions. These versions rely on some outdated dependencies that GitHub is likely going to be removing from their runners soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Should have used a more up to date tutorial 😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, do you mean I should use v4 instead of v2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay things should be good now. Now i just need to get the tests passing lol

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, do you mean I should use v4 instead of v2?

The latest version of setup-python is actually v5. v4 uses a newer but still EOL version of Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay. I thought I saw v4 in the docs.

@AndKaminer
Copy link
Contributor Author

For some reason when I run the tests, the pre-commit fails on windows, but not linux. All the other tests pass. Not sure what's up.
I would put the output in here, but it's really long. Does whoever wrote the precommit config have any idea why that is?

@aphedges
Copy link
Contributor

For some reason when I run the tests, the pre-commit fails on windows, but not linux. All the other tests pass. Not sure what's up. I would put the output in here, but it's really long. Does whoever wrote the precommit config have any idea why that is?

I was one of the people who worked on adding pre-commit, and both of us use Macs as dev machines, so we never encountered the problem on Windows. If you want help, you need to post the error. You can use the instructions on Organizing information with collapsed sections - GitHub Docs to include the error without it taking up too much space.

Without you saying anything, I suspect that your problem is with mixed-line-endings because Git changes line endings by default on Windows. You'll need to figure out how to override that during the checkout process.

@AndKaminer
Copy link
Contributor Author

For some reason when I run the tests, the pre-commit fails on windows, but not linux. All the other tests pass. Not sure what's up. I would put the output in here, but it's really long. Does whoever wrote the precommit config have any idea why that is?

I was one of the people who worked on adding pre-commit, and both of us use Macs as dev machines, so we never encountered the problem on Windows. If you want help, you need to post the error. You can use the instructions on Organizing information with collapsed sections - GitHub Docs to include the error without it taking up too much space.

Without you saying anything, I suspect that your problem is with mixed-line-endings because Git changes line endings by default on Windows. You'll need to figure out how to override that during the checkout process.

Yes you're exactly right. I've been looking at it for the past little while. Sorry @aphedges ! I need to actually do a good hard look before posting. I went ahead and fixed the issue with mixed-line-endings, and now there's an issue with shellcheck. I'm taking a look at it now.

@AndKaminer
Copy link
Contributor Author

Apparently shellcheck only works on linux when run through github actions. Not sure what to do about that. I'd like to tell it to run pre-commit with (SKIP=shellcheck) if it's running on windows and just precommit if on linux, but I can't seem to find a good way to do that.

@aphedges
Copy link
Contributor

This fix looks like it will work: actions/checkout#135. That way, you don't need to change mixed-line-ending.

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
@AndKaminer
Copy link
Contributor Author

This fix looks like it will work: actions/checkout#135. That way, you don't need to change mixed-line-ending.

@aphedges That worked. Thank you! Sorry I couldn't figure that out myself.

@AndKaminer
Copy link
Contributor Author

Okay, I think this is everything good now. I went ahead and added mac-os support to testing. I'll open up another PR to get rid of the deprecated methods in the example programs.

@MarcCote
Copy link
Collaborator

Looks good to me. I see all tests are passing on your repo's actions. Should be good. 👍

@MarcCote MarcCote merged commit f4d67d2 into allenai:main Dec 18, 2023
1 check passed
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.

None yet

3 participants