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

Organize requirements.txt #92

Merged
merged 3 commits into from
Aug 23, 2022
Merged

Organize requirements.txt #92

merged 3 commits into from
Aug 23, 2022

Conversation

iamEAP
Copy link
Member

@iamEAP iamEAP commented Aug 3, 2022

What / Why

Python dependencies are hard. Especially when the package you're publishing is sort of a weird hybrid between "library" (where one should not pin things!) and an end-product (where reproducibility is very important, and therefore one should definitely pin things).

This change doesn't actually change any of the resolved dependencies (as of today). But it attempts to organize things into logical groups:

  1. Dependencies that define compatible operating environments (e.g. mkdocs and markdown)
  2. Dependencies that we reflect back out as features of our own package (e.g. mdx sane lists)
  3. Dependencies that are like category 1, except they're only in place temporarily to work around an underlying python dependency issue. (We no longer have these, but we were carrying around a couple for a long time see chore(deps): remove dependency pyparsing #91 & chore(deps): update dependency jinja2 to v3.1.2 #78)

So that we can more easily reason about dependency updates from renovate.

Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
@iamEAP iamEAP requested a review from a team as a code owner August 3, 2022 13:47
Copy link
Contributor

@camilaibs camilaibs 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 arranging the dependencies in a way that helps us review the changes here, especially for the pinned group, which are critical dependencies.

iamEAP added 2 commits August 3, 2022 17:27
Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
plantuml-markdown==3.5.1
markdown_inline_graphviz_extension>=1.1.1,<2.0
markdown_inline_graphviz_extension==1.1.1
Copy link
Member

Choose a reason for hiding this comment

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

I see that markdown_inline_graphviz_extension>=1.1.1,<2.0 was introduced as part of this PR #74 after this comment #74 (review)

@iswariyam I talked to @iamEAP and we're a little concerned about the open-endedness of themarkdown_inline_graphviz_extension version constraint...
How about combining what we did with the markdown constraint >=1.1.1,<2 or something?

Before we merge this I just want to make sure this is not still a concern?

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 think moving the dependency from an extract version to a range was mistaken advice on my part in that PR. The hope is that splitting out the dependencies into these categories (as done in this PR) will help our future selves reason about ranges vs. exact versions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree with that, just wanted to double check with you both around that specific comment! So lets 🚢 it!

plantuml-markdown==3.5.1
markdown_inline_graphviz_extension>=1.1.1,<2.0
markdown_inline_graphviz_extension==1.1.1
Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree with that, just wanted to double check with you both around that specific comment! So lets 🚢 it!

@emmaindal emmaindal merged commit 6ca19b8 into main Aug 23, 2022
@iamEAP iamEAP deleted the dependency-sanity branch August 23, 2022 11:52
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.

4 participants