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

refactor test_create_to_ical from test_encoding to test_examples.py #439

Conversation

jacadzaca
Copy link
Collaborator

Refactor test_create_to_ical, test_create_event_simple and test_unicode_parameter_name from test_encoding along the lines of #374, move them to test_examples.py.

@@ -0,0 +1,41 @@
'''tests ensuring that *the* way of doing things works'''
Copy link
Member

Choose a reason for hiding this comment

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

The docstring does not explain anything.

Copy link
Member

@niccokunzmann niccokunzmann left a comment

Choose a reason for hiding this comment

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

I do not know what to do with it. I would need to reconnect to the value of refactoring this. It seems to be an ok refactoring. It can be merged as I have nothing against it except from the docstring. I mean in short: it does not hurt and I have nothing against it.

@niccokunzmann niccokunzmann merged commit 6be9960 into collective:master Oct 7, 2022
@jacadzaca
Copy link
Collaborator Author

I think creating a single file with tests that showcase practical icalendar usage is important, so as to expand the examples section. We know that documentation is somewhat lacking in examples, because there have been issues where people ask how to do X, e.g. #317, #341, #317, #421, #425.

@niccokunzmann
Copy link
Member

@jacadzaca, could you create an issue for this? I would find it nice if

  1. The documentation with the examples shows up on readthedocs
  2. That the code is actually running.

As a suggestion, one could use doctest
I do think that this issue is a very good first-timer issue because of its repetitiveness, the simplicity and the range of skills you build up while contributing to it. I would really love to see it. If you do not like to make this issue, I will do it.

What do you think?

@jacadzaca
Copy link
Collaborator Author

I'm off to bed now, but I'll gladly file it tomorrow :)

@jacadzaca
Copy link
Collaborator Author

@niccokunzmann is #443 OK?

@niccokunzmann
Copy link
Member

niccokunzmann commented Oct 9, 2022 via email

@niccokunzmann
Copy link
Member

niccokunzmann commented Oct 9, 2022 via email

@jacadzaca
Copy link
Collaborator Author

Feel free to edit it :)

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.

2 participants