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 issue where passing two styles would fail with index error #162

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jspaezp
Copy link
Contributor

@jspaezp jspaezp commented Sep 10, 2022

Fixes the first section of:

Closes #160

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@codecov
Copy link

codecov bot commented Sep 10, 2022

Codecov Report

Merging #162 (edf4976) into main (9268fd6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #162   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          591       592    +1     
=========================================
+ Hits           591       592    +1     
Impacted Files Coverage Δ
pydocstringformatter/_formatting/base.py 100.00% <100.00%> (ø)
...stringformatter/_formatting/formatters_numpydoc.py 100.00% <100.00%> (ø)

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 10, 2022

Tests fail due to the auto-fix from pre-commit.ci 0c6795b
... which raises the question ... why is it generating those trailing whitespaces?

  • Should a "two pass" approach be done?
  • Should there be an "Extras" where everything that was not placed in a section would go?

@DanielNoord
Copy link
Owner

  • Should a "two pass" approach be done?

I have been thinking about this as well, but not sure if we should do that as we can create infinite loops.

Those trailing whitespaces seem to match the previous indentation. I think that is a bug and should only be a new line without the extra spacing.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 12, 2022

@DanielNoord What about this behavior? Where there is a section separator but no name. I think it makes clear that the parser was not able to assign what is under it to any section and not that it is part of the former.

Those trailing whitespaces seem to match the previous indentation. I think that is a bug and should only be a new line without the extra spacing.

@github-actions

This comment has been minimized.



def sincos(theta):
"""Returns.
Copy link
Owner

Choose a reason for hiding this comment

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

Oh this is becoming more difficult than I hoped. It seems both styles are somewhat incompatible with each other. This is the missing section name, which is now being used as a summary/title...

I'll have to think about this. Perhaps we should fix this in another PR, but I like to check if this is easily fixable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well ... if you believe that the issue is that the styles are mutually exclusive in some instances, enforcing that is not hard internally.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the issue that they are currently mutually exclusive. Perhaps we can fix this by changing the order in which formatters are called. I hope to have time somewhere this week to look into this.

@jspaezp
Copy link
Contributor Author

jspaezp commented Sep 16, 2022

@DanielNoord

  • Should a "two pass" approach be done?

I have been thinking about this as well, but not sure if we should do that as we can create infinite loops.

Those trailing whitespaces seem to match the previous indentation. I think that is a bug and should only be a new line without the extra spacing.

Thinking about this ... I started working a branch that does a two pass to check the stability of the tokens.
I will reference it in a new issue.

@Pierre-Sassoulas
Copy link
Collaborator

Was this replaced by #169 @jspaezp ? (Should we close ?)

@jspaezp
Copy link
Contributor Author

jspaezp commented May 2, 2023

Hello @Pierre-Sassoulas! sorry for not closing this issue before (just for the future, it was fixed here #163)

@jspaezp jspaezp closed this May 2, 2023
@Pierre-Sassoulas
Copy link
Collaborator

No problem, thank you for answering :) !

@DanielNoord
Copy link
Owner

DanielNoord commented May 2, 2023

I do think this test still fails on main? At least it did last time I checked after we merged #163.

Edit: this does indeed still reproduce.

@github-actions
Copy link

According to the primer, this change has no effect on the checked open source code. 🤖🎉

@DanielNoord DanielNoord marked this pull request as draft May 15, 2023 22:03
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.

Bug, Unhandled case when a section is empty
3 participants