Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Organised the functions into seperate files #2

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

AvaterClasher
Copy link
Contributor

@AvaterClasher AvaterClasher commented Mar 16, 2024

  • Organised the functions into seperate files.
  • Solved the other issues pointed in the pr like output file, tsdoc changes, new line before } .
  • Added Doc comments to all the functions

…em in the main file

Solved the other issues pointed in the pr like output file, tsdoc changes, new line before }
This provides clear clarification about what you can currently expect from the project and how to run it
@dteare
Copy link
Contributor

dteare commented Mar 17, 2024

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
Copy link
Contributor

dteare commented Mar 17, 2024

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
Copy link
Contributor Author

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

@dteare
Copy link
Contributor

dteare commented Mar 17, 2024

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
Copy link
Contributor Author

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.

Copy link
Contributor

@dteare dteare left a comment

Choose a reason for hiding this comment

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

Very happy to see the main.rs monolith being broken up. 👍

@dteare dteare merged commit 1d1979a into Kindness-Works:main Mar 18, 2024
@dteare
Copy link
Contributor

dteare commented Mar 18, 2024

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants