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

Cleanup #8

Open
wants to merge 63 commits into
base: trunk
Choose a base branch
from
Open

Cleanup #8

wants to merge 63 commits into from

Conversation

bandophahita
Copy link

@bandophahita bandophahita commented Oct 12, 2023

Attempting to breakup #6 into smaller chunks
This PR does the following:

  • moves each package example into sub folders that avoids namespace collisions
  • adds pyproject.toml
  • add ruff and basic rules (additional rules will follow once screenpy is updated)
  • fixes some outdated tests (mostly locators)
  • adds logging

@bandophahita
Copy link
Author

This PR addresses mostly structural cleanup, linting, etc. The couple exceptions to that is to fix the existing tests and introduce clean logging for users to witness the outcome.

@bandophahita bandophahita marked this pull request as ready for review October 12, 2023 17:38
@perrygoy
Copy link
Member

I think i know what's rubbing me wrong about this PR. It treats the top-level as the package here.

This repo was originally a monorepo for all of the examples, where each individual folder had examples of using one of the ScreenPy extensions. It doesn't make sense to me to have a pyproject.toml at the top level because that's not what this was meant to do, in my mind.

If there were a pyproject.toml in each of the example directories, maybe. But i don't personally imagine this being published and pip install-able, and i think it might cause headaches to try to treat this monorepo-of-many-separate-projects as a single project.

What are your thoughts on that?

@bandophahita
Copy link
Author

bandophahita commented Feb 15, 2024

While it's not a repo that would get published, having each "sub-project" at the root level causes issues with namespacing; especially if there are imports that can be shareable between them. i.e, I created a logger that is used by more than one of the examples. It also causes problems when trying to use linting tools. Moving them down a folder solved that issue.

As for a pyproject.toml, keep in mind is it's not only meant for package installation, but also dependency management. Trunk doesn't have any requirements.txt or setup.py so users have to figure out for themselves what packages to install if they want to see it run. I figured I would add some documentation explaining they can just use pip install -e .[dev] and run whichever given example.

I also have to wonder if each of them should be separated into their own repos for the reasons you mentioned. If that is the case, does it maybe make sense to move them into a subfolder of their package repo?

screenpy_selenium
|_ docs
|_ screenpy_selenium
|_ tests
|_ examples
  |_ github
  |_ the_internet

If examples are exclusively limited to a given package it should be fairly simple. However, if there are examples that show of cross package usage then it feels like it would make more sense to put that into a separate repo.

From a maintenance standpoint it would be a pain to have more than one repo for examples (assuming their not combined with their counterpart packages)

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