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

Add rfc documentation #730

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Firdous2307
Copy link
Contributor

@Firdous2307 Firdous2307 commented Oct 13, 2024

RFC 5545 Documentation Addition

This merge request adds motivational text from RFC 5545 to the following calendar components:

  • Alarm
  • Event
  • Todo
  • FreeBusy
  • Timezone (including subclasses TimezoneStandard and TimezoneDaylight)
  • Calendar

Changes

The added documentation provides a brief description of each component's purpose and functionality, as defined in the RFC. This change:

  1. Improves the overall documentation of the library
  2. Helps developers better understand the purpose of each component

Timezone Component

Special attention was given to the Timezone component and its subclasses (TimezoneStandard and TimezoneDaylight) to accurately represent their roles in defining time zone information.

Note

The Journal component was already documented and served as an example for the other components.


These changes address issue #726 and enhance the codebase's alignment with the RFC specifications.

# Example of added documentation
class Event(Component):
    """
    An "VEVENT" calendar component is a grouping of component
    properties that represents a scheduled amount of time on a
    calendar. For example, it can be an activity; such as a one-hour
    long, department meeting from 8:00 AM to 9:00 AM, tomorrow.
    """
    # ... existing code ...

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

@Firdous2307
Copy link
Contributor Author

@niccokunzmann Kindly check my PR

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11317751379

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 97.337%

Totals Coverage Status
Change from base Build 11317448813: 0.001%
Covered Lines: 3501
Relevant Lines: 3593

💛 - Coveralls

@@ -148,3 +148,16 @@ This section contains useful links for maintainers and collaborators:

- `Future of icalendar, looking for maintainer #360 <https://github.com/collective/icalendar/discussions/360>`__
- `Comment on the Plone tests running with icalendar <https://github.com/collective/icalendar/pull/447#issuecomment-1277643634>`__

Copy link
Member

Choose a reason for hiding this comment

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

Could you create a new PR without the old code?

You can look up how to

  • create a new branch
  • use git cherry-pick to move the commits over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am confused, can you explain better?

Copy link
Member

Choose a reason for hiding this comment

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

Sure! When I look at the commits here:

https://github.com/collective/icalendar/pull/730/commits

It seems that you added the commits of the other PR into this PR.
That is not what is usually done on GitHub: Each PR should be able to stand alone, be merged and reviewed alone.

There is this one commit here:
grafik

That is the only one that should go into this PR. Basically this PR should be based on the main branch of collective/icalendar and go to the main branch of collective/icalendar with one additional commit, nothing else.

This way, we do not get confused and re changes only show up in one PR and do not need addressing in different placed. This is also an old library, so it is good to keep a bit of discipline because we might need to know why changes happened 10 years later.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to do this:

git checkout a27b146f50c1ddd83117302372d0704ce8ff0853 # the commit
git checkout -b new-branch-for-PR
git push -u origin new-branch-for-PR

And create a new PR without the other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay @niccokunzmann I understand now, I'll work on that.

Copy link
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.

Minor English grammar fixes, and remarks on change log.

Copy link
Member

Choose a reason for hiding this comment

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

You need to merge main into your branch to update this file. 6.0.1 was released 5 hours ago.

Then add your changes that this PR makes to this file under the correct version heading and topic subheading.

2. `tox.ini`: Update the `envlist` to include or remove the Python version.
3. `pyproject.toml`: Update the `requires-python` line and the `classifiers` list.
4. `README.rst`: Update the compatibility information.
5. `docs/maintenance.rst`: Update this list if any new files need to be modified.
Copy link
Member

Choose a reason for hiding this comment

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

For readability, I would convert this enumerated list to a definition list, and remove : after each term. See https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lists-and-quote-like-blocks

Also inline literals in reStructuredText should use double backticks, not single as in Markdown.

@@ -549,6 +549,12 @@ def is_datetime(dt: date) -> bool:
return isinstance(dt, datetime)

class Event(Component):
"""
An "VEVENT" calendar component is a grouping of component
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An "VEVENT" calendar component is a grouping of component
A "VEVENT" calendar component is a grouping of component

"""
An "VEVENT" calendar component is a grouping of component
properties that represents a scheduled amount of time on a
calendar. For example, it can be an activity; such as a one-hour
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
calendar. For example, it can be an activity; such as a one-hour
calendar. For example, it can be an activity, such as a one-hour

An "VEVENT" calendar component is a grouping of component
properties that represents a scheduled amount of time on a
calendar. For example, it can be an activity; such as a one-hour
long, department meeting from 8:00 AM to 9:00 AM, tomorrow.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
long, department meeting from 8:00 AM to 9:00 AM, tomorrow.
long department meeting from 8:00 AM to 9:00 AM tomorrow.

@@ -693,6 +699,13 @@ def end(self, end: date | datetime | None):


class Todo(Component):
"""
A "VTODO" calendar component is a grouping of component
properties that represents an action-item or assignment. For
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
properties that represents an action-item or assignment. For
properties that represents an action item or assignment. For

A "VTODO" calendar component is a grouping of component
properties that represents an action-item or assignment. For
example, it can be used to represent an item of work assigned to
an individual; such as "Prepare for the upcoming conference
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
an individual; such as "Prepare for the upcoming conference
an individual, such as "Prepare for the upcoming conference

@niccokunzmann niccokunzmann mentioned this pull request Oct 13, 2024
2 tasks
@niccokunzmann
Copy link
Member

@Firdous2307 How is it going, how are you? Let me know if you need anything or have questions how to go on. GitHub is not always easy.

@Firdous2307
Copy link
Contributor Author

Firdous2307 commented Oct 15, 2024

@Firdous2307 How is it going, how are you? Let me know if you need anything or have questions how to go on. GitHub is not always easy.

@niccokunzmann Thanks for the help, I have created a new branch and cherry picked the specific commit. I want to push a new commit to that new branch and fix the mistake english spellings

@Firdous2307
Copy link
Contributor Author

@niccokunzmann Should I just fix the Mistake spellings and the suggestion @stevepiercy made on the docs/maintenanc.rst on this branch. Then the new branch will be only for that specific commit as stated earlier.

I think that will help me reduce work instead of merging this branch with the new one, let me know wht you think

@niccokunzmann
Copy link
Member

@Firdous2307, I think that you could close the PR and create a new one. The grammar fixes pointed out can be applied to the files in the new PR manually on your computer. Is that good for you?

@Firdous2307
Copy link
Contributor Author

@Firdous2307, I think that you could close the PR and create a new one. The grammar fixes pointed out can be applied to the files in the new PR manually on your computer. Is that good for you?

@niccokunzmann Yeah sure no problem.

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.

4 participants