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
@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.

@ManiMozaffar
Copy link
Contributor Author

Screenshot 2024-06-03 at 22 53 48

Rebased again, still happens, still can't reproduce it :/
What this is actually checking? @sydney-runkle

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.

@ManiMozaffar,

Thanks for your initial work on this. A few notes:

  • Let's make sure that the pyproject.toml files only have information relevant to this project
  • I want to make sure we don't include any behavioral changes, just linting fixes and settings / dependency management
  • We might want to bump to the latest ruff version now that some new ones have been released since this PR's opening.

Ping me when you'd like another review 🚀!

Comment on lines +35 to +39
"fastapi>=0.110.2",
"httpx>=0.27.0",
"openai==1.10.0",
"PYjwt>=2.8.0",
"pydantic[email]>=2.7.1",
Copy link
Member

Choose a reason for hiding this comment

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

Are these all truly dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's not the pyproject for fastui, that's for the demo.
Yes they're all. but it shouldn't affect anyway, because demo itself is "dev" package right? it's not that we ship it to client.

Comment on lines +45 to +63
line-length = 120
target-version = 'py38'
lint.extend-select = ['Q', 'RUF100', 'C90', 'UP', 'I', 'D', 'T']
lint.extend-ignore = [
'D100',
'D101',
'D102',
'D103',
'D104',
'D105',
'D107',
'D205',
'D415',
'T201',
]
lint.flake8-quotes = { inline-quotes = 'single', multiline-quotes = 'double' }
lint.isort = { known-first-party = ['pydantic', 'tests'] }
lint.mccabe = { max-complexity = 14 }
lint.pydocstyle = { convention = 'google' }
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make sure these are consistent with what we're using in pydantic in general - seems like a lot of ignores?

quote-style = 'single'

[tool.coverage.run]
source = ['pydantic']
Copy link
Member

Choose a reason for hiding this comment

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

This seems off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true my bad, i tried to copy from pydantic's rule, some were dead codes.

Comment on lines +66 to +77
'docs/*' = ['D']
'pydantic/__init__.py' = ['F405', 'F403', 'D']
'tests/test_forward_ref.py' = ['F821']
'tests/*' = ['D']
'pydantic/deprecated/*' = ['D']
'pydantic/json_schema.py' = ['D']

[tool.ruff.lint.extend-per-file-ignores]
"docs/**/*.py" = ['T']
"tests/**/*.py" = ['T', 'E721', 'F811']
"tests/benchmarks/test_fastapi_startup_generics.py" = ['UP006', 'UP007']
"tests/benchmarks/test_fastapi_startup_simple.py" = ['UP006', 'UP007']
Copy link
Member

Choose a reason for hiding this comment

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

Same here - we don't need these in this repo

Comment on lines +150 to +157
user = users[id - 1] if id <= len(users) else None
if user is None:
return demo_page(
*tabs(),
c.Text(text='User not found.'),
title='User not found',
)

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we don't have any behavioral changes in this PR - just linting fixes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but that's also on demo. It doesn't affect the real package.
Side note -> this change was made to fix type check issues Otherwise i'd have to use # type: ignore

description = 'FastUI demo project'
authors = [{ name = 'Samuel Colvin', email = 's@muelcolvin.com' }]
classifiers = [
'Development Status :: 5 - Production/Stable',
Copy link
Member

Choose a reason for hiding this comment

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

Is it? Haha

Comment on lines +65 to +77
[tool.ruff.lint.per-file-ignores]
'docs/*' = ['D']
'pydantic/__init__.py' = ['F405', 'F403', 'D']
'tests/test_forward_ref.py' = ['F821']
'tests/*' = ['D']
'pydantic/deprecated/*' = ['D']
'pydantic/json_schema.py' = ['D']

[tool.ruff.lint.extend-per-file-ignores]
"docs/**/*.py" = ['T']
"tests/**/*.py" = ['T', 'E721', 'F811']
"tests/benchmarks/test_fastapi_startup_generics.py" = ['UP006', 'UP007']
"tests/benchmarks/test_fastapi_startup_simple.py" = ['UP006', 'UP007']
Copy link
Member

Choose a reason for hiding this comment

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

Same thing, we'll want to clean these files up

@elephantum
Copy link

For completeness I think this link belongs here: astral-sh/rye#1164 (comment)

Seems like rye would be mostly in maintenance mode

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.

5 participants