Skip to content

Fix docstring style in parser module#1111

Open
Himanshu-Vishwakarma-GH wants to merge 4 commits intocollective:mainfrom
Himanshu-Vishwakarma-GH:fix-docstring-style
Open

Fix docstring style in parser module#1111
Himanshu-Vishwakarma-GH wants to merge 4 commits intocollective:mainfrom
Himanshu-Vishwakarma-GH:fix-docstring-style

Conversation

@Himanshu-Vishwakarma-GH
Copy link
Copy Markdown

@Himanshu-Vishwakarma-GH Himanshu-Vishwakarma-GH commented Jan 29, 2026

Closes issue

Replace ISSUE_NUMBER with the issue number that your pull request fixes. Then GitHub will link and automatically close the related issue.

  • Closes #ISSUE_NUMBER

Description

Write a description of the fixes or improvements.

Checklist

  • I've added a change log entry to CHANGES.rst.
  • I've added or updated tests if applicable.
  • I've run and ensured all tests pass locally by following Run tests.
  • I've added or edited documentation, both as docstrings to be rendered in the API documentation and narrative documentation, as necessary.

Additional information

Upload screenshots, videos, links to documentation, or any other relevant information.


📚 Documentation preview 📚: https://icalendar--1111.org.readthedocs.build/

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 29, 2026

Pull Request Test Coverage Report for Build 21797239153

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.975%

Totals Coverage Status
Change from base Build 21796449300: 0.0%
Covered Lines: 11161
Relevant Lines: 11386

💛 - Coveralls

@niccokunzmann
Copy link
Copy Markdown
Member

@Himanshu-Vishwakarma-GH Thanks for your PR! Can you fill out the description of the PR at the top?

Before we merge this, we need to fix the tests... That seems like some work. I also wonder if vale should run on the python files. It could be that it is better to let it run on the output of the built html files - or can it run on docstrings only?
https://github.com/collective/icalendar/actions/runs/21466632585/job/61830072927?pr=1111

Comment thread docs/how-to/usage.rst Outdated
Comment thread docs/how-to/usage.rst Outdated
Comment thread docs/how-to/usage.rst Outdated
Comment thread PR_CHANGES.md
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Before merging this PR, this file should be removed. It's good to keep it for now, while we troubleshoot the PR and try to understand what's not passing.

Comment thread docs/how-to/usage.rst Outdated
@stevepiercy
Copy link
Copy Markdown
Member

@Himanshu-Vishwakarma-GH sorry for the multiple posts. Although there are a lot of spelling errors now reported by Vale, many of them are repeated tens or hundreds of times. It shouldn't be too difficult to resolve.

See https://icalendar.readthedocs.io/en/latest/contribute/documentation/build-check.html#spelling-grammar-and-style for tips and tricks.

Most of the spelling errors appear to be of Python objects. Changing the vale.ini configuration might reduce the false positives by ignoring them.

Please ask if you have any question about how each error should be resolved. These can be tricky, and it's easy to go down the wrong path.

Copy link
Copy Markdown
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Fixing test

Comment thread docs/how-to/usage.rst Outdated
@stevepiercy
Copy link
Copy Markdown
Member

@niccokunzmann I've pushed this forward as far as I care to do at this time. If @Himanshu-Vishwakarma-GH doesn't respond within a reasonable time, then feel free to close this PR as abandoned.

I think it would be better to run over the rendered HTML than the Python files. The Vale errors are so numerous on Python files, that I don't think it's worth going down this path.

https://github.com/collective/icalendar/actions/runs/21797239154/job/62886736238

@Himanshu-Vishwakarma-GH
Copy link
Copy Markdown
Author

I'll fix that don't worry @stevepiercy

@niccokunzmann
Copy link
Copy Markdown
Member

@Himanshu-Vishwakarma-GH Thanks for the help. I think you contributed in a helpful way! We were not sure how we should have approached this issue and it needed someone to step in and go for it so we can evaluate this. You did and it turns out that the approach is not working to spell-check Python files. So you succeeded and we know now that we should look for another way to spell-check the docstrings! There is no need to push this fix forward at the moment from our side. Feel free to experiment of cause! But yeah, thanks for putting the work in so that we have more clarity now!

Feel free to close this if you want, I think you did a good job :)

@niccokunzmann
Copy link
Copy Markdown
Member

Is there an issue that we can link this in?

@stevepiercy
Copy link
Copy Markdown
Member

@niccokunzmann #1007.

I'm still not sure whether rendered HTML or checking Python source is better, as this method does show considerable promise. Let's see if @Himanshu-Vishwakarma-GH can whittle down the errors to something more manageable. Right now it's overwhelming!

Also note the prop module reorganization in #1150. Because of this, I'd avoid making any changes to Python source files at this time. For Python source file changes, we should make a plan before making all the changes that Vale suggests. It's OK to make any changes to Vale configuration, of course, as it's only reporting issues.

@stevepiercy stevepiercy added the doc label Feb 9, 2026
@niccokunzmann
Copy link
Copy Markdown
Member

Hi, we created a new release, please move your edits in CHANGES.rst to the new section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants