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

⛔ Remove pypiwin32 package #62

Merged
merged 3 commits into from
Jan 9, 2024
Merged

⛔ Remove pypiwin32 package #62

merged 3 commits into from
Jan 9, 2024

Conversation

SimmonsRitchie
Copy link
Contributor

@SimmonsRitchie SimmonsRitchie commented Jan 9, 2024

Summary

Deletes pypiwin32 from our Pipfile and rebuilds our lock file, which appears to solve a dependency bug that was causing our CI Github workflow to fail. This same fix was implemented for our city-scrapers repo.

This PR also:

  • requires Python 3.9 in our Pipfile.
  • Removes two top-level dependencies (twisted & chardet) in our Pipfile that aren't necessary

Why are we doing this?

In a recent PR I immediately experienced an identical dependency installation failure as described here in our city-scrapers repo. See that PR for further notes and discussion.

Requiring python 3.9 and the removal of chardet & twisted weren't necessary in order to resolve this issue but I felt it made sense to clean up the Pipfile while I was removing pypiwin32.

Are there any smells or added technical debt to note?

See the PR in city-scrapers for further notes and discussion.

Removing two packages from the project's Pipfile as they are already included as subdependencies by other top-level dependencies. I don't think we need to include them as top-level dependencies.
@SimmonsRitchie SimmonsRitchie changed the title 🐛 Fix: dependency install bug ⛔ Remove pypiwin32 package Jan 9, 2024
@SimmonsRitchie SimmonsRitchie marked this pull request as ready for review January 9, 2024 16:25
@SimmonsRitchie SimmonsRitchie requested a review from a team January 9, 2024 16:28
@SimmonsRitchie
Copy link
Contributor Author

@City-Bureau/pdw, I think since we were on the same page about this change in the city-scraper repo and it's pretty low-risk, I'm just going to cowboy merge this. I'm probably going to need to implement this fix for all 16 city-scraper repos, so it probably doesn't make sense for ya'll to review each change.

@SimmonsRitchie SimmonsRitchie merged commit 703f30d into main Jan 9, 2024
2 checks passed
@SimmonsRitchie SimmonsRitchie deleted the fix-deps branch January 9, 2024 17:29
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.

1 participant