[pydoclint] Fix false positive when Sphinx directives follow Raises section (DOC502)#20535
[pydoclint] Fix false positive when Sphinx directives follow Raises section (DOC502)#20535ntBre merged 6 commits intoastral-sh:mainfrom
pydoclint] Fix false positive when Sphinx directives follow Raises section (DOC502)#20535Conversation
|
| 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.
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.
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.
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.
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.
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