Skip to content

Conversation

tylerriccio33
Copy link
Contributor

This is a 2-fold PR:

  1. I created a group for css-inline package called css. This would be helpful because I often have to use the css inlining feature in ci, and i must document it as a dependency. However, carrying around selenium can be cumbersome in production, since it has some tough requirements of it's own.
  2. Reworked the _try_import function call out the dependency group needed to install said dependency instead of the generic pip install command. This would encourage users to follow best practices and add the correct group instead of one-off installing it.

I did these things which are not crucial but development quality of life. Feel free to strike them from the commit!

  • I added pre-commit commands in the Makefile so users can more easily install it.
  • Added uv.lock to the git ignore so it won't get tracked by users developing/contributing with uv.
  • Used uv to run the tests in the Makefile instead of generic pytest .... The old version would require users to activate the environment. Not sure if you want to keep this depending on your philosophy!

Copy link

codecov bot commented Aug 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.52%. Comparing base (654757a) to head (9755521).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #748      +/-   ##
==========================================
+ Coverage   91.45%   91.52%   +0.07%     
==========================================
  Files          47       47              
  Lines        5558     5557       -1     
==========================================
+ Hits         5083     5086       +3     
+ Misses        475      471       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rich-iannone
Copy link
Member

@tylerriccio33 we discussed this PR earlier today and came to the conclusion that splitting out css-inline into its own group would probably make things more complicated for the average user without a clear benefit. I can definitely understand that installing an unneeded selenium dependency makes no sense, but couldn't the alternative be installing css-inline separately (i.e., independent of installing great-tables)?

@tylerriccio33
Copy link
Contributor Author

Yea completely get that. The problem with having a separate install of css-inline is it usually fails CI, since it's not documented in the dependency tree and it's never imported, so the linters think it's an extra dependency. It's easy to override so not a big deal!

@tylerriccio33
Copy link
Contributor Author

I would argue though the _try_import refactor is worth a merge, since it specifically calls out what dep group you have to add. If you want to proceed with that I can edit the pr.

@rich-iannone
Copy link
Member

I would argue though the _try_import refactor is worth a merge, since it specifically calls out what dep group you have to add.

But if we’re not adding the second optional dependency group css then don’t we only have the extra group?

What does look pretty good in this PR are the additions to the Makefile. Would you be willing to close this PR and make a separate one with just the Makefile update?

@tylerriccio33
Copy link
Contributor Author

so even though it's just the extra, i still think it's beneficial to include that in the err message instead of it just saying 'run pip install '. In the change it would be 'run pip install[extra]' which is the more accurate install command.

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