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

Move to Python 3.11 baseline; increase blocklist import verbosity #63

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Aug 28, 2024

  • Bump Python version to 3.11
  • Updated dependencies require us to declare html-clean extra
  • We previously did not declare bs4 dependency even though it's used in our blocklist; this won't work in production. Added it to dependencies explicitly
  • Silently failing blocklist.py import as a result of the above was a bit annoying to debug; made the logic a bit more verbose so we can catch such issues more quickly in future
  • Provided an abstract base class for blocklists
  • Updated CI to use Python 3.11
  • Tweaked .gitignore to catch all pycache directories

Resolves #62

Copy link
Contributor

@harrislapiroff harrislapiroff left a comment

Choose a reason for hiding this comment

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

I offer one comment as food for thought, but am happy to provisionally approve this for your merge, once considered @eloquence

@@ -31,6 +31,8 @@ readability-lxml = "^0.8.1"
requests = "^2.31.0"
tweepy = "^4.14.0"
"Mastodon.py" = "^1.8.1"
lxml = {extras = ["html-clean"], version = "^5.3.0"}
beautifulsoup4 = "^4.12.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ambivalent about this. If it's not being used by the core script, it feels like this should be codified in the specific implementation that's using it instead. Like, ideally, our internal ttnconfig repo that includes the blocklist should have a separate requirements spec that lists both ttn and bs4. (If someone else wanted to use PyQuery for example, they'd still have to install that themselves.)

(Looks like I actually removed this requirement in the past for similar reasons heh 64bcb02)

On the other hand, this is a small project that's only used by a handful of people besides us, so I'd accept if it feels like too much bikeshedding to debate this. Probably not a big deal to include.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about dropping an abstract base class in this repo that does import bs4? Basically, more of an opinionated plugin-style design pattern intended to minimize the complexity folks may have to wrangle to add something like a basic blocklist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, sounds reasonable enough!

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll poke at this still in this PR, dragging back to WIP until that's done

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now done; I've avoided importing BeautifulSoup in the ABC since it serves no purpose there, but I've annotated pyproject.toml to make it clearer why it's there.

@eloquence
Copy link
Member Author

Back to you Harris; if this looks good, we'll need to land freedomofpress/foiafeed-ttnconfig#3 alongside it.

@harrislapiroff
Copy link
Contributor

We're still not using bs4 in this repo, which is what I thought you meant 😅 But I also don't need to overlitigate this one common dependency. It's good, let's merge!

@harrislapiroff harrislapiroff merged commit cdbe549 into main Sep 12, 2024
2 checks passed
@eloquence
Copy link
Member Author

I did mean that originally, but importing it in an abstract base class and then not using it there will just make the linter unhappy, so I went with the "comment the dep" approach instead.

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.

Python >=3.11 compat
2 participants