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.

Sign up for GitHub

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

Jump to bottom

Update cal.py #481

Merged
merged 22 commits into from Oct 31, 2022
Merged

Update cal.py #481

merged 22 commits into from Oct 31, 2022

Conversation

pronoym99
Copy link
Contributor

Convert format strings to python3.6+ compliant f-strings wherever possible.

@coveralls
Copy link

coveralls commented Oct 30, 2022 •
edited

Pull Request Test Coverage Report for Build 3358856911

  • 303 of 309 (98.06%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 93.046%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/icalendar/cal.py 303 309 98.06%
Totals Coverage Status
Change from base Build 3354567871: 0.006%
Covered Lines: 1164
Relevant Lines: 1251
💛 - Coveralls

@pronoym99
Copy link
Contributor Author

Please add the hacktoberfest label to this PR if possible.

@jacadzaca jacadzaca added the hacktoberfest Issues for participation in the hacktoberfest https://hacktoberfest.com/ label Oct 30, 2022
@jacadzaca jacadzaca mentioned this pull request Oct 30, 2022
Merged
@jacadzaca jacadzaca self-requested a review October 30, 2022 21:37
niccokunzmann
niccokunzmann requested changes Oct 31, 2022
View reviewed changes
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 am not willing to merge this PR for one reason: The 600 lines diff does not fit the purpose. Everyone else editing at the same time gests merge conflicts. We indent with 4 spaces. Please recreate the PR so that it changes the lines required and that being visible on the PR tab showing the files. Happy to merge it then.

niccokunzmann
niccokunzmann reviewed Oct 31, 2022
View reviewed changes
CHANGES.rst Outdated Show resolved Hide resolved
@niccokunzmann
Copy link
Member

Thanks for your PR! This is nice that it gets ported to later versions!

@niccokunzmann
Copy link
Member

I really liked the other PRs but I can not understand this one because I do not want to check over 1000 lines. I hope to merge it soon.

@pronoym99
Copy link
Contributor Author

I am not willing to merge this PR for one reason: The 600 lines diff does not fit the purpose. Everyone else editing at the same time gests merge conflicts. We indent with 4 spaces. Please recreate the PR so that it changes the lines required and that being visible on the PR tab showing the files. Happy to merge it then.

@niccokunzmann did changes in a way that properly renders a diff in the PR tab. Please have a look.

@pronoym99 pronoym99 requested review from niccokunzmann and removed request for jacadzaca October 31, 2022 08:57
angatha
angatha reviewed Oct 31, 2022
View reviewed changes
src/icalendar/cal.py Outdated Show resolved Hide resolved
@pronoym99 pronoym99 requested review from angatha and removed request for niccokunzmann October 31, 2022 15:24
@angatha
Copy link
Collaborator

angatha commented Oct 31, 2022

Thank you for your contribution and cleaning up the codebase.

Can you please also replace the str.format(...) call in src/icalendar/tools.py:33 (and the commented out one in src/icalendar/parser.py:264)? Then all these calls are replaced and we do not need a second pull request to get them all.

@pronoym99
Copy link
Contributor Author

@angatha, I have done the requested changes. Please have a look.

@angatha angatha merged commit 7b1f376 into collective:master Oct 31, 2022
@pronoym99 pronoym99 deleted the patch-1 branch October 31, 2022 18:02
@niccokunzmann
Copy link
Member

Thanks @pronoym99 and @angatha !

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

@niccokunzmann niccokunzmann niccokunzmann requested changes

@angatha angatha Awaiting requested review from angatha

Assignees
No one assigned
Labels
hacktoberfest Issues for participation in the hacktoberfest https://hacktoberfest.com/
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pronoym99 @coveralls @niccokunzmann @angatha @jacadzaca