This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add just and pre-commit #201
Add just and pre-commit #201
Changes from 16 commits
abf1f12
b24a10a
fa5d261
d2251db
1ef2741
974e572
98c4c32
abf3e52
58956d7
7bea78a
27a402c
30ccacc
46f0fba
ce352ec
d4e2682
79b4eef
caa41ee
ea9524b
728e4e5
f4c9023
c5192a0
45810ef
85f125b
0683180
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to match the official way their org is named.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their docs show a GitHub URL. Let's use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these args won't be necessary as the hook will automatically use the config from
.flake8
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't, unfortunately.
pre-commit
runs hooks inside a virtualenv that doesn't include the root of your project, so it doesn't know any of your configuration files 😢There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this locally and it seemed to work fine. Are you sure about it not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarayourfriend for the hook, is it possible to provide a config path instead of defining the rules twice in two different locations? If not I think just leaving a note here should be enough that mentions the rules also being in the other file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm not sure 😅 I'd tried it in the past and it didn't work and I found some SO answers about it to confirm the issue, but if you've got it working I'll try it again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it worked! I removed the duplicated config. Thanks for testing it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this by trying to run this command:
error: Justfile does not contain recipe `testlocal`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hmm I removed it for some reason. Let me see.
Do you know why these tests must run in a "pipenv subshell"?
Edit: Oh never mind, I see that the script calls to
pytest
.This is going to throw a wrench in the root Pipfile idea 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I run the tests inside the container, we can leave only those instructions for tests.
docker-compose exec web bash ./test/run_test.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's ever necessary to run it outside of the container 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes, I run the code outside the container when debugging it line by line (I like PyCharm's debugger GUI). Running the tests outside the containers helps in that scenario. I'm open to a different process though if I'm the only one using it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Pycharm can take the interpreter of a container, it's quite flexible in this aspect. However, I can see the value in the local approach in not being tied up to this particular IDE, neither to the use of
just
if that is what is blocking here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krysal from what I had read in the docs, PyCharm can work with a Docker environment only if it created the container. I wasn't able to get it to connect with the existing
web
container orchestrated by Docker Compose 😢.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think that's true Dhruv, the docs might be incorrect. At my previous job we were able to get it to connect to the docker container just fine and it was running using docker compose... but maybe things have changed since then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, after making the changes in the latest commits we're able to add back the
testlocal
script 🎉There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
testlocal
script works perfectly. So does the regulartest
one.