-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pydoclint] Fix false positive when Sphinx directives follow Raises section (DOC502)
#20535
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
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| DOC502 | 12 | 4 | 8 | 0 | 0 |
|
Note on
Net effect is reduced noise and improved precision |
ntBre
left a comment
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.
Thanks! I had an idea for a possibly simpler implementation more similar to the numpy version. Let me know what you think. We should keep a close eye on the ecosystem results to make sure they are still improved.
This new result actually looks incorrect to me:
- libs/langchain_v1/langchain/agents/react_agent.py:304:9: DOC502 Raised exception is not explicitly raised:
MultipleStructuredOutputsError
Maybe my suggestion will help with that too.
| }; | ||
| let entry = potential[..colon_idx].trim(); | ||
| entries.push(QualifiedName::user_defined(entry)); | ||
|
|
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.
What do you think about something more like this for this function?
fn parse_entries_google(content: &str) -> Vec<QualifiedName<'_>> {
let mut entries: Vec<QualifiedName> = Vec::new();
let mut lines = content.lines().peekable();
let Some(first) = lines.peek() else {
return entries;
};
let indentation = &first[..first.len() - first.trim_start().len()];
for potential in lines {
if let Some(entry) = potential.strip_prefix(indentation) {
if let Some(first_char) = entry.chars().next() {
if !first_char.is_whitespace() {
if let Some(colon_idx) = entry.find(':') {
let entry = entry[..colon_idx].trim();
if !entry.is_empty() {
entries.push(QualifiedName::user_defined(entry));
}
}
}
}
}
}
entries
}This is closer to the parse_entries_numpy down below and seems to work on the new test case when I tried it locally.
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 is significantly better, thank you! We will have to see what the ecosystem results will look like.
|
Could you take a look at the ecosystem results? It looks like there are still some issues. I think we may need a combination of the two approaches we've tried, especially breaking early if we find a dedented line like you had initially. We should extract some test cases from the erroneous ecosystem results too. |
|
Just to clarify, for the "dedented line" breaking logic, should it:
|
|
I think I was picturing option 1, possibly excluding completely blank lines, assuming those are allowed. I'm looking at
for example. The Returns:
str: The resulting expression for the start of the specified unit.
Raises:
ValueError: If the unit is not one of the valid options.
Relation to `time_range_lookup`:
- Handles the "start of" or "beginning of" modifiers in the first regex pattern.
- Example: "start of this month" → `DATETRUNC(DATETIME('today'), month)`.
"""But that's just an idea. |
ntBre
left a comment
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! This looks great and shows a very clear improvement in the ecosystem check.
I'm tempted to go ahead and land this, but I think this could also be a problem for numpy-style docstrings (example). These seem a little trickier since the exceptions are already dedented to the same level as the sphinx directive, assuming I followed the docs correctly. We may just have to check for a leading .. in that case.
Do you want to try fixing that here too, or would it be better as a separate PR?
|
I'll try to fix that here too, thanks! |
Updates the docstring parser to correctly terminate the numpy-style Raises section when encountering Sphinx directives (lines starting with '..'). Adds a regression test for this behavior to address issue astral-sh#18959.
ntBre
left a comment
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.
Thanks again!
I just pushed one commit moving the .. check into the indentation-stripped branch. I don't think this is likely to be common, but I could at least imagine a case like the new test I added where a Sphinx directive was used in the description of an exception, so the outer break would have been too eager.
Summary
Fixes #18959