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

Add rye and ruff, and local dependency for CI and development #287

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ManiMozaffar
Copy link
Contributor

@ManiMozaffar ManiMozaffar commented Apr 28, 2024

I'm going to write tests with playwright, but before doing that, I found repo a bit messy.

So I used PDM as it's conventional tool in pydantic team
Samuel asked to use rye instead.

I added some # type ignore, because this PR was not meant to fix all types.

about Playwright and ui test, since they usually take quite long time to pass, might make more sense to use "pre push hook" instead of pre commit? I find it very annoying to have some browser running on every commit :) (ofc I can ignore it, till last push, but that's also annoying hahaha)

From samuel input, which I also very agree with, we don't need to run playwright tests on any git hooks. Only in CI :) and also a way to execute it manually. Will engineer this in next PR

@ManiMozaffar ManiMozaffar changed the title Use PDM and local dependency Use PDM and local dependency for demo project Apr 28, 2024
@ManiMozaffar ManiMozaffar force-pushed the add-ui-tests branch 7 times, most recently from 26240c0 to 1c3d6cb Compare April 28, 2024 14:59
@ManiMozaffar ManiMozaffar marked this pull request as draft April 28, 2024 15:02
@ManiMozaffar ManiMozaffar force-pushed the add-ui-tests branch 2 times, most recently from 8b643dc to 7e3fac2 Compare April 28, 2024 15:04
@ManiMozaffar ManiMozaffar marked this pull request as ready for review April 28, 2024 15:04
@ManiMozaffar ManiMozaffar force-pushed the add-ui-tests branch 4 times, most recently from b9a46e3 to 130c0ee Compare April 28, 2024 15:23
Copy link

codecov bot commented Apr 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

Copy link
Contributor Author

@ManiMozaffar ManiMozaffar left a comment

Choose a reason for hiding this comment

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

Only lint ci fails for strange reason.

@ManiMozaffar
Copy link
Contributor Author

@sydney-runkle lmk if you agree with these changes ^ and your ideas about what i just said in my first comment so I can then start working on playwright tests :)

@samuelcolvin
Copy link
Member

hi @ManiMozaffar thanks so much for contributing.

I have a few thoughts:

  • please lets not use PDM, we use it on Pydantic due to the complex set of dependencies but I really don't love it - I'd suggest using rye, or sticking to what we have now
  • Playwright tests, while useful, can be pretty frustrating - let's keep the number to a minimum
  • let's run playwright tests on CI, and of course via a make command, no need for anything else

(btw, we don't really have a default packaging choice right now - pydantic uses PDM, pydantic-core uses pure pip, internal stuff uses poetry, things we're about to release use rye)

@samuelcolvin
Copy link
Member

Let's not switch package manager at the same time as adding playwright tests.

@ManiMozaffar
Copy link
Contributor Author

ManiMozaffar commented Apr 28, 2024

Hey samuel, thanks for quick check.

please lets not use PDM, we use it on Pydantic due to the complex set of dependencies but I really don't love it - I'd suggest using rye, or sticking to what we have now

Agreed. I was wondering why you're not using rye in pydantic. I can rework PR to use rye instead. Makes much more sense. That pip dependency was kinda bad IMO.

Playwright tests, while useful, can be pretty frustrating - let's keep the number to a minimum

I also asked Sydney some days ago, she also agreed to write test. Playwright is much better than selenium and other alternatives as for my experience, being frustrating depends on the what you test, and how much you test. Testing at correct amount with correct approach, wouldn't be frustrating to maintain. frustrating to run? maybe. that's why I was kinda against running it on pre commit hook.

Let's not switch package manager at the same time as adding playwright tests.

That's why I made this PR. To first decide about package manager, then continue with writing test. Sorry for the branch name, should have renamed it.

@ManiMozaffar ManiMozaffar marked this pull request as draft April 29, 2024 02:43
@ManiMozaffar ManiMozaffar force-pushed the add-ui-tests branch 3 times, most recently from 130c0ee to faa5697 Compare April 29, 2024 03:00
@ManiMozaffar ManiMozaffar changed the title Use PDM and local dependency for demo project Use rye and local dependency for demo project Apr 29, 2024
@ManiMozaffar ManiMozaffar marked this pull request as ready for review April 29, 2024 03:38
@ManiMozaffar
Copy link
Contributor Author

A bit more research into rye, found this:
https://lucumr.pocoo.org/2024/2/15/rye-grows-with-uv/

According to Armin Ronacher, rye and uv eventually converge into one. So the decision to choose rye might have not been the best there. But I think it might make more sense to just keep using rye for now as it's already there, and if in future rye converge into uv, migrate to uv.

@sydney-runkle
Copy link
Member

@ManiMozaffar,

Should we use uv instead?

@sydney-runkle
Copy link
Member

@ManiMozaffar,

I guess we're using a combo in https://github.com/pydantic/logfire, so we could always migrate to uv down the line.

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Hi!

Thanks for your contribution here. Happy to move towards using rye instead of the current messy pip system.

Left a few comments / change requests. I'd like to understand the need for the #type: ignore statements + see if we can fix the root cause!

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
demo/pyproject.toml Outdated Show resolved Hide resolved
demo/pyproject.toml Show resolved Hide resolved
demo/src/components_list.py Outdated Show resolved Hide resolved
src/python-fastui/fastui/auth/github.py Show resolved Hide resolved
src/python-fastui/fastui/forms.py Show resolved Hide resolved
src/python-fastui/pyproject.toml Outdated Show resolved Hide resolved
src/python-fastui/tests/test_auth_github.py Outdated Show resolved Hide resolved
@sydney-runkle
Copy link
Member

Ah also, looks like there are some changes with the __init__.py file with the components, likely due to my docs additions.

@ManiMozaffar
Copy link
Contributor Author

Ok great, I'll rebase the branch tomorrow and do the changes requested @sydney-runkle

@ManiMozaffar ManiMozaffar changed the title Use rye and local dependency for python projects Add rye and ruff, and local dependency in CI and development May 1, 2024
@ManiMozaffar ManiMozaffar changed the title Add rye and ruff, and local dependency in CI and development Add rye and ruff, and local dependency forCI and development May 1, 2024
@ManiMozaffar ManiMozaffar changed the title Add rye and ruff, and local dependency forCI and development Add rye and ruff, and local dependency for CI and development May 1, 2024
@ManiMozaffar
Copy link
Contributor Author

@sydney-runkle build docs fails. I don't have the "PPPR_TOKEN", so I can't test and debug it locally. I'm not even sure why it's failing, it said it requires black to function, so maybe pip install black would solve it? but then why didn't it happen before this PR? i think the diagnostic might be wrong.

@sydney-runkle
Copy link
Member

@sydney-runkle build docs fails. I don't have the "PPPR_TOKEN", so I can't test and debug it locally.

Ah yes, this is a barrier we need to work on. This is bc of the documentation PR that I merged recently. The local failure makes sense, but I'm not sure why it's failing on CI. I'll look into this!

@sydney-runkle
Copy link
Member

@ManiMozaffar,

Are you able to repro the lint failure locally at least?

@ManiMozaffar
Copy link
Contributor Author

@ManiMozaffar,

Are you able to repro the lint failure locally at least?

No
image

@sydney-runkle
Copy link
Member

What if you try make install first to make sure that your versions are lined up?

@sydney-runkle
Copy link
Member

Looks like the docs are failing on other PRs as well, so I need to fix that 🤧

@sydney-runkle
Copy link
Member

Will work on the docs fix tonight. Might end up disabling the typescript docs for now until mkdocstrings-typescript has a public version.

@sydney-runkle
Copy link
Member

@ManiMozaffar,

This should fix the docs issue (I'm hoping): #299.

@sydney-runkle
Copy link
Member

@ManiMozaffar,

You should be able to rebase now :).

@ManiMozaffar
Copy link
Contributor Author

@sydney-runkle still lint fails, not sure why. can't reproduce it locally.

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.

None yet

4 participants