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

♻️ Default to warning for missing needextend ID #1066

Merged
merged 3 commits into from
May 8, 2024

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Nov 7, 2023

Rather than defaulting to "strict" mode for unknown IDs in needextend directives, whereby the entire build is excepted,
the default is now changed to "non-strict" mode, and this emits a warning rather than an info log.

I would suggest that actually this deprecates the use of the strict config/directive option, since the user can already halt builds on warnings using the standard sphinx-build -W flag.

Closes #1025

@arwedus this would mean you could simply remove needs_needextend_strict = True and rely on the new default warning behaviour. Make sense?

@danwos
Copy link
Member

danwos commented Nov 7, 2023

I'm afraid this change destroys 1-2 use cases.

There are situations, where -W is already used, but a not found ID in needextend is okay, because the Need to extend is part of another Doc project or get imported via needimport, and this kind of separated data is not available during a local build or shall not be used, as it is too big and slows down the build too much.

So during a local build, not found needetend-IDs is fine. But during a CI build, which combines all sources, a missing ID must throw an error. And -W is used in all cases to identify problems quite quickly.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Nov 7, 2023

And -W is used in all cases to identify problems quite quickly.

I would note, with the warning added here, it has the needs.extend type/subtype, allowing you to "fine-grain" the use of -W with https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings

I'll have a think about your use case though

Perhaps one could even change the warning subtype, based on if it is strict or not, or something like that;
I'm certainly more in favour of solutions that do not completely except a build

@arwedus
Copy link
Contributor

arwedus commented Nov 7, 2023

@danwos: We cover that use case over our sphinx-warnings-filter extension instead of yet another config option for sphinx-needs. A [needs.extend] warning that can be configured in the sphinx warnings_filter list is imho indeed the cleanest solution.

@danwos danwos added this to the 2024.05 milestone May 2, 2024
@chrisjsewell chrisjsewell force-pushed the issue-1025-extend-unknown-need branch from b2b1bdd to 2e6689b Compare May 8, 2024 10:23
@chrisjsewell chrisjsewell force-pushed the issue-1025-extend-unknown-need branch from 2e6689b to eb2fd18 Compare May 8, 2024 10:23
Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 86.33%. Comparing base (42962cc) to head (dce60e1).

Files Patch % Lines
sphinx_needs/directives/needextend.py 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
- Coverage   86.41%   86.33%   -0.09%     
==========================================
  Files          56       56              
  Lines        6531     6535       +4     
==========================================
- Hits         5644     5642       -2     
- Misses        887      893       +6     
Flag Coverage Δ
pytests 86.33% <75.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chrisjsewell chrisjsewell merged commit d405fd1 into master May 8, 2024
19 checks passed
@chrisjsewell chrisjsewell deleted the issue-1025-extend-unknown-need branch May 8, 2024 11:33
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.

Sphinx-needs throws an exception when extending unknown need
3 participants