-
Notifications
You must be signed in to change notification settings - Fork 3
Add CI job to run lint (tests will come later) and resolve lint issues #7
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments for reviewers; most of the changes are done by ruff; I left comments for my manual changes
- name: Lint with ruff | ||
run: make lint | ||
|
||
# TODO: enable this once all the tests pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the tests still fail, so I will come up with a separate PR for enabling this
- **Install from this repo:** | ||
```bash | ||
pip install -e .[presidio] | ||
pip install -e '.[presidio]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be removed soon, but I noticed it does not work zsh + macOs for me
|
||
import asyncio | ||
import os | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this kind of changes are done by ``uv run ruff check --unsafe-fixes --fix`
"Programming Language :: Python :: 3", | ||
"Programming Language :: Python :: 3.11", | ||
"Programming Language :: Python :: 3.12", | ||
"Programming Language :: Python :: 3.13", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 3.13 because we should be fine with the latest major too
[tool.ruff] | ||
line-length = 100 | ||
target-version = "py39" | ||
line-length = 150 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed but fine to adjust if we want to; 100 is too small
] | ||
isort = { combine-as-imports = true, known-first-party = ["guardrails"] } | ||
extend-ignore=[ | ||
"D100", # Missing docstring in public module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't want to ignore these, we need to add more comments; we can do it later though
convention = "google" | ||
|
||
[tool.ruff.lint.extend-per-file-ignores] | ||
"tests/**" = ["E501"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were so many warnings due to this in tests, but they should not be critical
|
||
# Type alias for OpenAI response types | ||
OpenAIResponseType = Union[Completion, ChatCompletion, ChatCompletionChunk, Response] | ||
OpenAIResponseType = Union[Completion, ChatCompletion, ChatCompletionChunk, Response] # noqa: UP007 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manually added; we can resolve this but I intentionally didn't change code manually this time
# This persists across turns to maintain multi-turn context | ||
# Only used when a guardrail in _NEEDS_CONVERSATION_HISTORY is configured | ||
_user_messages: ContextVar[list[str]] = ContextVar('user_messages', default=[]) | ||
_user_messages: ContextVar[list[str]] = ContextVar('user_messages', default=[]) # noqa: B039 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want to improve this, we need to set None for the default value and have a method to set an empty list afterwards
- 0.0 = Certain not hallucinated | ||
- Use the full range [0.0 - 1.0] to reflect your level of certainty | ||
""" | ||
""" # noqa: E501 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't repeat this for the rest but I've added this manually
The job is failing due to lack of uv on the platform. I will resolve it shortly but if you have any comments, please let me know! |
OK, the uv issue resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these!
This pull request adds the following things:
uv run ruff check --unsafe-fixes --fix
and added ignore options and comments to clear everything