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

fix line number handling for errors and warnings #1150

Closed
wants to merge 19 commits into from
Closed

fix line number handling for errors and warnings #1150

wants to merge 19 commits into from

Conversation

PhilipPartsch
Copy link
Contributor

With this PR I try to fix #1077

@PhilipPartsch PhilipPartsch changed the title add test to show wrong line number output proper error and warning line no handling Mar 22, 2024
@PhilipPartsch PhilipPartsch changed the title proper error and warning line no handling proper line number handling for errors and warnings Mar 22, 2024
@PhilipPartsch PhilipPartsch changed the title proper line number handling for errors and warnings fix line number handling for errors and warnings Mar 22, 2024
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

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

Project coverage is 85.97%. Comparing base (90bae4a) to head (9279233).

❗ Current head 9279233 differs from pull request most recent head 4b65a07. Consider uploading reports for the commit 4b65a07 to get more accurate results

Files Patch % Lines
sphinx_needs/api/need.py 95.65% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
+ Coverage   85.92%   85.97%   +0.04%     
==========================================
  Files          56       56              
  Lines        6536     6558      +22     
==========================================
+ Hits         5616     5638      +22     
  Misses        920      920              
Flag Coverage Δ
pytests 85.97% <96.00%> (+0.04%) ⬆️

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.

@danwos
Copy link
Member

danwos commented Mar 24, 2024

Thanks for the PR and the great fix.
It looks good to me, but I haven't tested it locally.
And I'm also not an expert with docutils.
So @chrisjsewell, do you have any concerns?

Also thanks a lot for updating the tests and adding a new one 👍

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

The general concept looks good thanks 👍, but see review comments for implmentation details

PhilipPartsch and others added 4 commits March 25, 2024 09:34
Changed sphinx version detection to explicit extract from sphinx version as suggested in review.
@PhilipPartsch
Copy link
Contributor Author

@chrisjsewell why did you have removed the content_offset from the needs.json?
In #1082 @danwos wrote:

Customized handling (fast prototyping)
This use case is for me the main reason, why needs.json got chosen.
shall be complete
shall be easy to understand and use
Shall work without any changes or configuration

So I would want to bring in the content_offset, and even work on to bring back the _back links #1157 and add the lineno #1156.

@chrisjsewell and @danwos: How do we want to proceed?

@chrisjsewell
Copy link
Member

chrisjsewell commented Apr 12, 2024

why did you have removed the content_offset from the needs.json

Heya, because as I have already mentioned, including full source-mapping in the needs.json is a separate concern to this particular PR, and it would not currently make sense to include it anyway since the line number is not included so you have nothing to offset against

@PhilipPartsch
Copy link
Contributor Author

why did you have removed the content_offset from the needs.json

Heya, because as I have already mentioned, including full source-mapping in the needs.json is a separate concern to this PR, and it would not currently make sense to include it anyway since the line number is not included so you have nothing to offset against

I fought we agreed to add this information to the needs.json. Even I try to add the lineno. Anyway, the content_offset is usefull without the lineno, as it is the offset of the directive content from the beginning of the file.

@chrisjsewell
Copy link
Member

I fought we agreed to add this information to the needs.json

No that is not the case: #1156 (comment), again I'm not against doing this perse, but it needs to be considered/implemented outside of this PR which is specifically for fixing warnings

as it is the offset of the directive content from the beginning of the file

no that is not the case; it is the offset from the top of the directive

@PhilipPartsch PhilipPartsch deleted the proper_error_warning_line_no branch April 19, 2024 17:28
@PhilipPartsch PhilipPartsch restored the proper_error_warning_line_no branch April 19, 2024 17:41
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.

nested_parse with line offset
3 participants