-
-
Notifications
You must be signed in to change notification settings - Fork 175
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 from_file and to_file method #757
base: main
Are you sure you want to change the base?
Conversation
A few questions:
|
376e391
to
804ab52
Compare
Pull Request Test Coverage Report for Build 12468792178Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @abe-101, I left a few comments. What are your thoughts?
>>> from icalendar import Calendar | ||
>>> # Read a calendar file | ||
>>> cal = Calendar.from_file("src/icalendar/tests/calendars/example.ics") | ||
>>> # Read multiple calendars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the >>> here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggestion to remove the >>>
on the lines where there is a comment? or perhaps just remove the comments altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I remove the >>>
from the comments then the test fails
This doesn't work:
Example:
>>> from icalendar import Calendar
>>> from pathlib import Path
# Read a calendar file
>>> cal = Calendar.from_file("src/icalendar/tests/calendars/example.ics")
# Read multiple calendars
>>> cals = Calendar.from_file("src/icalendar/tests/calendars/multiple_calendar_components.ics", multiple=True)
# or pass a Path object
>>> path = Path("src/icalendar/tests/calendars/example.ics")
>>> cal = Calendar.from_file(path)
This does work:
Example:
>>> from icalendar import Calendar
>>> from pathlib import Path
>>> # Read a calendar file
>>> cal = Calendar.from_file("src/icalendar/tests/calendars/example.ics")
>>> # Read multiple calendars
>>> cals = Calendar.from_file("src/icalendar/tests/calendars/multiple_calendar_components.ics", multiple=True)
>>> # or pass a Path object
>>> path = Path("src/icalendar/tests/calendars/example.ics")
>>> cal = Calendar.from_file(path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs a new line:
>>> from pathlib import Path
# Read a calendar file
Otherwise the comment is considered output.
src/icalendar/cal.py
Outdated
>>> from icalendar import Calendar | ||
>>> from pathlib import Path | ||
>>> # Read a calendar file | ||
>>> cal = Calendar.from_file("src/icalendar/tests/calendars/example.ics") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples here seem to be the same as above. You can use a temporary file for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the examples to use the to_file()
method
Hi @abe-101, thanks for your PR!
Yes, if they are behind a
No, I think a proper API documentation is enough for now.
If you feel like it, a new PR might be the right place for that. Let's finish this one... |
|
||
Example: | ||
>>> from icalendar import Calendar | ||
>>> # Read a calendar file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> # Read a calendar file | |
# Read a calendar file |
Hi, could you write a test for that? |
Sure. |
7e8eb10
to
7002bb6
Compare
Hi, thanks! I think that the doctests are not enough. What needs to happen in testing is this:
For now, not having any code in the method would also pass the doctests. |
closes #756
📚 Documentation preview 📚: https://icalendar--757.org.readthedocs.build/