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

Document vRecur property #759

Merged
merged 10 commits into from
Dec 22, 2024

Conversation

fluxxcode
Copy link
Contributor

@fluxxcode fluxxcode commented Dec 21, 2024

This PR adds an example to the usage documentation on how to add an RRULE from a string.

Closes #758


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

@niccokunzmann
Copy link
Member

Hi, this is nice... I wonder why it fails. I will have a look.

In the mean time, could you also add these examples to the vRecur class documentation?

docs/usage.rst Outdated

>>> event.add('rrule', {'freq': 'daily', 'interval': 10})

>>> event.add('rrule', vRecur.from_ical('FREQ=DAILY;INTERVAL=10'))
Copy link
Member

@niccokunzmann niccokunzmann Dec 21, 2024

Choose a reason for hiding this comment

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

Oh, I found it... We actually run the documentation code to make sure it works.
You add another RRULE entry and that makes the output different a few lines further down.

I think, a good place to add the documentation for this is actually the class string:

"""Recurrence definition.

There is almost nothing and more is appreciated! Could you add these two examples to the class string? Could you remove this line causing duplicate entry?


This is another issue to make this class nicer to understand:
#742
That is - if you like this class and making it pretty ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I copied most of the docstring from the rfc. Let me know if you want me to adjust anything.

docs/usage.rst Outdated Show resolved Hide resolved
@niccokunzmann
Copy link
Member

niccokunzmann commented Dec 21, 2024

@fluxxcode Have a look, can you see the tests?

https://github.com/collective/icalendar/actions/runs/12449350281/job/34754863315#step:5:192

(I am not sure who Github allows to see them)

@niccokunzmann
Copy link
Member

Nice!

@fluxxcode
Copy link
Contributor Author

My code editor removed trailing whitespaces from other docstrings. Let me know if I should revert the diffs

@fluxxcode fluxxcode changed the title Add vRecur example to usage documentation Document vRecur property Dec 21, 2024
@stevepiercy
Copy link
Member

Direct links to preview changes:

@@ -1100,6 +1100,68 @@ def __reduce_ex__(self, _p):

class vRecur(CaselessDict):
"""Recurrence definition.

Property Name: RRULE
Copy link
Member

Choose a reason for hiding this comment

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

All parameter names must appear on their own line, followed by an optional description beginning on an indented new line. Please adjust the others accordingly. Except is OK. See current rendering at https://icalendar--759.org.readthedocs.build/en/759/api.html#icalendar.prop.vRecur

Suggested change
Property Name: RRULE
Property Name:
RRULE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rendering now looks good

@coveralls
Copy link

coveralls commented Dec 22, 2024

Pull Request Test Coverage Report for Build 12449529289

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

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 96.372%

Totals Coverage Status
Change from base Build 12449452667: 0.0%
Covered Lines: 4580
Relevant Lines: 4747

💛 - Coveralls

@niccokunzmann niccokunzmann merged commit 03e32e9 into collective:main Dec 22, 2024
13 of 14 checks passed
@niccokunzmann
Copy link
Member

Thanks! This is definitely a huge improvement!

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.

Add rrule from a string
4 participants