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

Correct dev requirements path in cache key #64

Merged
merged 1 commit into from
May 27, 2022
Merged

Correct dev requirements path in cache key #64

merged 1 commit into from
May 27, 2022

Conversation

rlskoeser
Copy link
Contributor

I noticed the cache key had the wrong filename for the dev requirements.

Although I can't tell if the dev requirements are being used anywhere... should they be?

@digitaldogsbody
Copy link
Member

Although I can't tell if the dev requirements are being used anywhere... should they be?

This is because setuptools doesn't support a development requirements construct so we moved them to be separate because they are only needed for development (specifically for linting and running tests), and we didn't want to make them a hard requirement of the library.

@digitaldogsbody
Copy link
Member

Some discussion of that here: #53

@digitaldogsbody digitaldogsbody requested a review from a team May 27, 2022 15:16
Copy link
Contributor

@glenrobson glenrobson left a comment

Choose a reason for hiding this comment

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

I think this would be good to merge. I think the original dev-requirements.txt is a copy and paste error on my part so would be good to fix. It means the gitHub action will get create a new cache if we update either the setup.py file or the dev requirements file.

@glenrobson glenrobson added this to Ready for review in Hackathon May 27, 2022
@rlskoeser
Copy link
Contributor Author

This is because setuptools doesn't support a development requirements construct so we moved them to be separate because they are only needed for development (specifically for linting and running tests), and we didn't want to make them a hard requirement of the library.

fwiw, you can use named extras for things like dev/test dependencies in setup (not that you need to necessarily use that here)

example:
https://github.com/Princeton-CDH/parasolr/blob/main/setup.py#L15-L21
https://github.com/Princeton-CDH/parasolr/blob/main/setup.py#L38-L42

and then you can install with pip install -e '.[dev]'

@glenrobson glenrobson requested a review from a team May 27, 2022 15:48
@digitaldogsbody
Copy link
Member

fwiw, you can use named extras for things like dev/test dependencies in setup (not that you need to necessarily use that here)

example: https://github.com/Princeton-CDH/parasolr/blob/main/setup.py#L15-L21 https://github.com/Princeton-CDH/parasolr/blob/main/setup.py#L38-L42

and then you can install with pip install -e '.[dev]'

It did cross my mind to see if it could be done like this, but I didn't have time to investigate it fully (my experience with setuptools is a little limited). I've opened #73 so that we can refactor it this way in the future, thanks!

@digitaldogsbody digitaldogsbody merged commit 10a800b into iiif-prezi:main May 27, 2022
@digitaldogsbody digitaldogsbody moved this from Ready for review to Done in Hackathon May 27, 2022
@glenrobson
Copy link
Contributor

Still need to move requirements.dev.txt into setup.py.

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

Successfully merging this pull request may close these issues.

None yet

3 participants