Infrastructure improvements worth discussing #888
Replies: 3 comments
-
|
@SashankBhamidi thanks for your feedback. Would you please create an issue and pull request for Contributing workflow gaps? I feel comfortable reviewing and merging a PR that makes it easier for new contributors. I'll leave the rest for @niccokunzmann and others to comment on. |
Beta Was this translation helpful? Give feedback.
-
|
#672 speaks about the formatting. Indeed, lots can be formatted. If you like, contribute to the conversation there and push it further. Many files are already formatted. I think, we could use a PR that formats them all. I will be absent for about a month. Feel free to address these changes in that time, as there will be less PRs open to add code, causing merge conflicts. For me, the goal of good formatting can be tested like this:
My goal is that it is easy for new contributors to format the code properly. At the moment, I only run the pre-commit on the files I change. However, now could be a perfect time to format the whole code base or even more of it. What do you think? Would you like to take this on? Feel free to improve the documentation on contributing! For code duplication, I suggest that you create an issue linking the lines and methods in a checklist for all the duplications you find you find worth working on. That allows for clear closing criteria and you can create the PRs referencing the issue and the issue can be closed when done. Yes, thanks for smoothing things with tests. @SashankBhamidi, I invited you to become a collaborator. That allows you to merge other people's pull requests, too and your own if you wait for a week to two weeks without anyone giving a review or merge - the purpose is to increase progress and invite contribution, improving the code together rather than waiting for perfection alone. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @stevepiercy and @niccokunzmann for the clear direction! @stevepiercy I'll create an issue and PR for the contributing workflow improvements - making the CHANGES.rst requirement more visible for new contributors. @niccokunzmann I'll check out #672 first to understand the existing approach, then work on the codebase-wide ruff formatting PR while you're away. The pre-commit test method makes sense - I'll use that to ensure we're completely formatted. For the code duplication, I'll create a separate issue with a checklist of specific patterns I found - that'll make it easier to tackle systematically. Thanks for the collaborator invite! Looking forward to helping move things forward. I'll start with the contributing docs since that's the quickest win, then move on to the formatting work. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Hey @niccokunzmann and maintainers,
I've been working on several PRs (#879, #880, #881, #882, #887) and noticed some things that might be worth addressing project-wide.
The ruff situation
Running
ruff checkon main shows 601 violations. Most are theOptional[X]vsX | Nonething (201 fixable ones), but there's a mix of other formatting issues too.Here's the thing - when I was working on my PRs, it made it harder to see what I actually changed vs what was just formatting drift. Not a huge deal, but it adds noise.
Contributing workflow gaps
The CHANGES.rst requirement tripped me up initially. My first PR failed the changelog check because I missed it. Maybe worth making that more visible in the contributing docs.
Code duplication I found
While working on the setter methods, I kept running into duplicate logic between Event and Todo classes. PR #887 fixes the
set_durationduplication, but there are similar patterns elsewhere. Validation logic, property handling, stuff like that.Testing gaps
I found several bugs just by writing comprehensive tests. The existing test suite is good, but some edge cases weren't covered. That's how I discovered the Todo.duration bug without DTSTART.
What I'm wondering
Would you want help tackling the formatting stuff? I could do it incrementally - maybe start with the
Optionalmigration since that's the biggest chunk.Also curious about your thoughts on the code duplication patterns. Worth consolidating or is the current approach preferred?
No pressure either way. Just wanted to bring it up since I kept bumping into these things while contributing.
Beta Was this translation helpful? Give feedback.
All reactions