Skip to content

requirements.txt file added #969

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

Closed
wants to merge 4 commits into from
Closed

Conversation

ZEUS-03
Copy link
Contributor

@ZEUS-03 ZEUS-03 commented Apr 17, 2023

closes: #964

Checklist

Please update this checklist as you complete each item:

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.
  • GitHub Issues which may be closed by this Pull Request have been linked.

@rmorshea
Copy link
Collaborator

rmorshea commented Apr 17, 2023

I don't think this is quite right. Two things:

  1. What we actually want is for all dependencies to be pinned. So not just the build-docs.txt deps as mentioned here, but also the runtime dependencies. The goal is to avoid breakages that might occur when releases of our dependencies happen.
  2. The pinned dependencies actually needs to be installed in the Dockerfile running the docs. On its own a .txt file with dependencies doesn't actually do anything unless you install them.

@rmorshea
Copy link
Collaborator

rmorshea commented Apr 17, 2023

Additionally, can you restore the original PR template? It's useful for maintainers to be able to see what has/hasn't been done yet.

@ZEUS-03
Copy link
Contributor Author

ZEUS-03 commented Apr 17, 2023

I don't think this is quite right. Two things:

  1. What we actually want is for all dependencies to be pinned. So not just the build-docs.txt deps as mentioned here, but also the runtime dependencies. The goal is to avoid breakages that might occur when releases of our dependencies happen.
  2. The pinned dependencies actually needs to be installed in the Dockerfile running the docs. On its own a .txt file with dependencies doesn't actually do anything unless you install them.

Yeah sounds good!

@ZEUS-03
Copy link
Contributor Author

ZEUS-03 commented Apr 17, 2023

Additionally, can you restore the original PR template? It's useful for maintainers to be able to see what has/hasn't been done yet.

How do i restore the previous template? if you have any idea...

@rmorshea
Copy link
Collaborator

I guess it's alright to leave as-is this time. The main thing from the checklist that you'd need to do is link the associated issue by adding closes: #123 where 123 is the issue number.

In the future, you shouldn't delete PR templates. Other projects may simply close your PRs as spam if you don't follow their contribution guidelines.

@ZEUS-03
Copy link
Contributor Author

ZEUS-03 commented Apr 17, 2023

I guess it's alright to leave as-is this time. The main thing from the checklist that you'd need to do is link the associated issue by adding closes: #123 where 123 is the issue number.

In the future, you shouldn't delete PR templates. Other projects may simply close your PRs as spam if you don't follow their contribution guidelines.

I got it. Thanks!

@ZEUS-03 ZEUS-03 marked this pull request as ready for review April 17, 2023 18:20
Copy link
Contributor Author

@ZEUS-03 ZEUS-03 left a comment

Choose a reason for hiding this comment

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

Proposed changes completed

@ZEUS-03 ZEUS-03 marked this pull request as draft April 18, 2023 15:24
@ZEUS-03 ZEUS-03 requested a review from rmorshea April 18, 2023 15:25
@ZEUS-03 ZEUS-03 marked this pull request as ready for review April 18, 2023 16:52
@ZEUS-03
Copy link
Contributor Author

ZEUS-03 commented Apr 18, 2023

Updated packages list. Do tell if this resolves the issue!

@rmorshea
Copy link
Collaborator

Closing as there's been a change of plans.

@rmorshea rmorshea closed this Apr 23, 2023
@ZEUS-03 ZEUS-03 deleted the gaush#964 branch April 24, 2023 10:40
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.

Use poetry to pin docs dependencies
2 participants