search

Kindness-Works / rocket-ts

Generate types directly from Rocket request handlers
MIT License
2 stars 1 forks source link

Organised the functions into seperate files #2

Closed AvaterClasher closed 7 months ago

AvaterClasher commented 7 months ago
dteare commented 7 months ago

It's great seeing you working on another PR!

I wanted to review your changes and get this merged, but I see you have included a task list in your description to indicate that this is a Work In Progress. Could you use the Draft Pull Requests feature instead so myself and others know when you're ready for a review?

I don't want to be too picky as I love having help here, but I know from our email correspondence that you'd like to learn how best to work within a team environment so I'm going to be more prescriptive than I usually would be on an open source project.

So with that in mind, it would be nice to have more focused PR that would allow for targeted discussions about individual changes and faster merges. Right now there's so much going on that it's more difficult to review. And you're not done yet so it'll only get harder. 🙂

For example, your first task listed in this PR would be a quick and easy review. And really, those targeted changes have nothing to do with the stated goal of this PR. As it stands now it's really hard to see these changes with all of the reorganization taking place here.

Another example is I'd like to discuss example.md as I don't see the point of having this file when the README.md already included this. I'm guessing it's because you're planning to use it for your tests, but it's hard to have this discussion when there is so much else going on in this PR.

I think a lot of this will naturally get easier as this project matures but in general it would be nice to bring more focus to each PR and making it clear when you're ready for review. I should also open issues to improve my communication as well. 🙂

dteare commented 7 months ago

I opened #4. Along with the README.md I think they would eliminate the need for example.ts.

I also opened #3 which I think you might enjoy taking a crack at.

AvaterClasher commented 7 months ago

So should i drop this pr and open some smaller pr's tackling each issue ?

dteare commented 7 months ago

So should i drop this pr and open some smaller pr's tackling each issue ?

That would be best but it's work so I'd totally understand if you want to keep it as-is. Given it's so early in the project I'm happy either way.

AvaterClasher commented 7 months ago

Made it a little bit easier for you to review (I guess) removed the example.md and example.ts files. You can merge this pr for the other issues I will open other smaller pr's 😅 that are easier to review. Thank you again.

dteare commented 7 months ago

Thank you! And thank you for your willingness to embrace more focused PRs. 💜