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.

Sign up for GitHub

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

Jump to bottom

core: Add N(naming) ruff rules #25362

Merged
merged 2 commits into from
Sep 19, 2024
Merged

core: Add N(naming) ruff rules #25362

merged 2 commits into from
Sep 19, 2024

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Aug 13, 2024 •
edited
Loading

Public classes/functions are not renamed and rule is ignored for them.

@vercel Vercel
Copy link

vercel bot commented Aug 13, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Sep 19, 2024 5:06am

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. langchain Related to the langchain package 🤖:refactor A large refactor of a feature(s) or restructuring of many files labels Aug 13, 2024
cbornet
cbornet commented Aug 13, 2024
View reviewed changes
libs/core/langchain_core/output_parsers/xml.py
except ImportError:
raise ImportError(
"defusedxml is not installed. "
"Please install it to use the defusedxml parser."
"You can install it with `pip install defusedxml`"
"See https://github.com/tiran/defusedxml for more details"
)
_ET = DET # Use the defusedxml parser
_et = defusedxml.ElementTree # Use the defusedxml parser
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This variable doesn't seem to be used...

Copy link
Member

Choose a reason for hiding this comment

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

@eyurtsev should this be used below in the fromstring?

@eyurtsev eyurtsev self-assigned this Aug 14, 2024
@eyurtsev
Copy link
Collaborator

@cbornet how difficult was this PR to generate? Is it relying on any auto-fixing behavior from Ruff? I'm worried about merge conflicts for 0.3 release since we're maintaining a separate branch for it and there may be merge conflicts

@cbornet
Copy link
Collaborator Author

cbornet commented Aug 14, 2024

how difficult was this PR to generate? Is it relying on any auto-fixing behavior from Ruff? I'm worried about merge conflicts for 0.3 release since we're maintaining a separate branch for it and there may be merge conflicts

Not very hard. I didn't use auto-fixing since sometimes we can't change the name for various reasons.
I was not aware of the 0.3 branch. I can delay this PR or apply it to the 0.3 branch instead if you prefer.

@ccurme ccurme added Ɑ: core Related to langchain-core and removed langchain Related to the langchain package labels Aug 14, 2024
@eyurtsev
Copy link
Collaborator

@cbornet I'll see if I can merge this one or not. And we can do other linting changes once 0.3 is out

@cbornet cbornet changed the title core: Add N(naming) ruff rules [core] Add N(naming) ruff rules Aug 21, 2024
@cbornet cbornet changed the title [core] Add N(naming) ruff rules core: Add N(naming) ruff rules Aug 22, 2024
@cbornet
Copy link
Collaborator Author

cbornet commented Sep 17, 2024

@eyurtsev I've rebased after 0.3 merge.
Can you take a look ?

@eyurtsev
Copy link
Collaborator

@cbornet hi it looks good -- but it needs another remerge since it picked up merge conflict from the other ruff change PR that was merged yesterday

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Sep 18, 2024
@efriis efriis enabled auto-merge (squash) September 19, 2024 05:06
@efriis efriis merged commit fd21ffe into langchain-ai:master Sep 19, 2024
98 checks passed
@cbornet cbornet deleted the ruff-core-n branch September 19, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efriis efriis efriis left review comments

@eyurtsev eyurtsev eyurtsev approved these changes

Assignees

@eyurtsev eyurtsev

Labels
Ɑ: core Related to langchain-core lgtm PR looks good. Use to confirm that a PR is ready for merging. 🤖:refactor A large refactor of a feature(s) or restructuring of many files size:L This PR changes 100-499 lines, ignoring generated files.
Projects
PR Reviews
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cbornet @eyurtsev @efriis @ccurme