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

🐛 Fix dependency install bug #19

Merged
merged 12 commits into from
Dec 26, 2023
Merged

🐛 Fix dependency install bug #19

merged 12 commits into from
Dec 26, 2023

Conversation

SimmonsRitchie
Copy link
Contributor

@SimmonsRitchie SimmonsRitchie commented Dec 20, 2023

Summary

Updates our Pipfile to fix a dependency conflict issue that has caused our scheduled Github action to fail for the past two months. It also:

  • Requires Python 3.9 in our Pipfile
  • Specifies Python 3.9 in our Github workflows
  • Re-formats five files based on the style rules of a newer version of Black
  • Removes three packages that appear to be unused (chardet, twisted and pypiwin32)

Why are we doing this?

We want working scrapers! These changes are modeled heavily on the Pipfiles of our others repos, particularly city-scrapers, which are functioning properly.

These changes appear to have achieved their goal: both our CI and cronjob workflows are now working.

How can these changes be manually tested?

  1. If you've previously installed this project, remove your existing virtual environment:
    pipenv --rm
  2. Reinstall the project and ensure that there are no errors:
    pipenv sync --dev
  3. Run a few of our checks to simulate the checks we run in our CI process:
    pytest && black . --check
  4. Ensure our custom Scrapy command is working properly and doesn't throw any errors:
    scrapy validate

For extra credit, you can check that the cronjob workflow works as expected by manually triggering it from this page and using the dropdown menu (pictured below). Just note that it will take about an hour to run:

image

Are there any smells or added technical debt to note?

  • I've specified the Python venv as 3.9 because I think it's ideal for our Github workflows and our dev environment to use the same environment. I've observed a number of dependency conflicts in this and our other city-scraper repos (eg. city-scrapers) and I think it will make these repos easier to maintain and easier to debug in future. I welcome feedback on this choice.
  • pypiwin32 is among one of the packages removed. I believe this package was originally installed in order to help Windows-users set up their dev environment. My understanding is this package is outdated and we don't need it. I have more thoughts and research notes in a separate PR here.

@SimmonsRitchie SimmonsRitchie changed the title :bug Fix dependency install bug 🐛 Fix dependency install bug Dec 20, 2023
@SimmonsRitchie SimmonsRitchie requested a review from a team December 20, 2023 23:02
@SimmonsRitchie SimmonsRitchie requested review from a team and removed request for a team December 20, 2023 23:09
@SimmonsRitchie SimmonsRitchie marked this pull request as ready for review December 21, 2023 16:51
Pipfile Outdated
black = "*"

[requires]
python_version = "3.9"

Choose a reason for hiding this comment

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

@SimmonsRitchie you can configure your IDE to automatically add a new line at the end of file on save, so Github won't show the red error icon above. See more here https://thoughtbot.com/blog/no-newline-at-end-of-file

Copy link
Contributor Author

@SimmonsRitchie SimmonsRitchie Dec 26, 2023

Choose a reason for hiding this comment

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

Nice catch and great tip! Thanks, @LienDang. This should be fixed now 😊

@LienDang
Copy link

Looks good to me!

@SimmonsRitchie
Copy link
Contributor Author

Excellent! Thanks, @LienDang! Merging now 🚀

@SimmonsRitchie SimmonsRitchie merged commit dee6161 into main Dec 26, 2023
2 checks passed
@SimmonsRitchie SimmonsRitchie deleted the fix-deps2 branch December 26, 2023 16:17
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.

2 participants