search

OurTechCommunity / catchup

The OTC CatchUp web app and workflows.
https://catchup.ourtech.community
MIT License
15 stars 9 forks source link

:truck: update: Add 150th CatchUp summary #189

Closed mohitgangwani closed 11 months ago

mohitgangwani commented 11 months ago

Requesting a review!

mohitgangwani commented 11 months ago

Sorry # was not supposed to be there, a small typo. I fixed the needed.

SirusCodes commented 11 months ago

@mohitgangwani I have added fixes you can check them later

HarshKapadia2 commented 11 months ago

@mohitgangwani I have added fixes you can check them later

Hi @SirusCodes, thank you for making some of the changes! Moving forward, I think it would be better for us to review summaries instead of directly pushing changes, especially when people are trying to learn our way of documenting things! Reviews provide a better learning experience in my opinion, as we have to explain our thinking, which helps out the person new to our way of doing things with some context on why things are the way they are!

HarshKapadia2 commented 11 months ago

Also @mohitgangwani, our summary addition commit messages usually follow the format :truck: update: Add 150th CatchUp summary. Just something to keep in mind going further!

SirusCodes commented 11 months ago

@mohitgangwani I have added fixes you can check them later

Hi @SirusCodes, thank you for making some of the changes! Moving forward, I think it would be better for us to review summaries instead of directly pushing changes, especially when people are trying to learn our way of documenting things! Reviews provide a better learning experience in my opinion, as we have to explain our thinking, which helps out the person new to our way of doing things with some context on why things are the way they are!

Agreed but there were a lot of small changes and anyways anyone can see the diff later. The whole thing had to be reformated so I felt pushing would be better.

HarshKapadia2 commented 11 months ago

@mohitgangwani I have added fixes you can check them later

Hi @SirusCodes, thank you for making some of the changes! Moving forward, I think it would be better for us to review summaries instead of directly pushing changes, especially when people are trying to learn our way of documenting things! Reviews provide a better learning experience in my opinion, as we have to explain our thinking, which helps out the person new to our way of doing things with some context on why things are the way they are!

Agreed but there were a lot of small changes and anyways anyone can see the diff later. The whole thing had to be reformated so I felt pushing would be better.

I realised that there are a lot of small changes, but the point is to make the other person make all those changes so that they actively realise what they're changing, have the chance to question it (which might make us realise our stupidity as well) and it's more effective learning than just looking at someone else's code.

mohitgangwani commented 11 months ago

Hello, I have made changes, let me know if I have missed any. Except the doubt which I had asked earlier in this thread, I guess everything is up to date as per the required changes.

@HarshKapadia2 @SirusCodes

SirusCodes commented 11 months ago

LGTM

HarshKapadia2 commented 11 months ago

@mohitgangwani, I wanted to ask why you force pushed your catchup-summary-150 branch.

mohitgangwani commented 11 months ago

Nothing in particular if I remember correctly.

HarshKapadia2 commented 11 months ago

Okay. I would recommend not doing it without a concrete reason.

If you did it because you thought the commit history would look messy, be assured that we will squash all commits into one commit before merging (using the 'Squash and merge' option that GitHub has).

Having all commits visible here (in the PR) makes it easy to (re)view changes and see the development of the code over time or can make it easy to go to a particular change.

mohitgangwani commented 11 months ago

Will keep this in mind, @HarshKapadia2. I'll make sure to look for everything in next summary. Thankyou so much for pointing things out. I'm learning alot!

Also, thanks @SirusCodes for the changes, I appreciate it!

HarshKapadia2 commented 11 months ago

No problem! Thank YOU for writing the summary!

KartikSoneji commented 11 months ago

@HarshKapadia2 @mohitgangwani force pushing is fine on a feature/summary branch like this if you really feel the need to rewrite the commit history. Sometimes you should force push, especially if you have undone and redone changes, otherwise the diff will show the effected lines without any changes, which is a bit confusing. Also if you want to rewrite a commit message, or have accidentally committed a large file to the repo that shouldn't be there. So in general try to avoid force pushing but you should use it when you feel it's required.