Skip to content

Conversation

khvr1993
Copy link

@khvr1993 khvr1993 commented Jul 8, 2022

I will be adding solutions one by one as I go through the questions. This is a work in progress PR

@khvr1993 khvr1993 marked this pull request as draft July 11, 2022 06:09
@khvr1993 khvr1993 marked this pull request as ready for review July 13, 2022 15:01
@khvr1993
Copy link
Author

Hi @Maximilian5189 Could you review this and if you have write access request you to merge the PR.
Thank you

Copy link
Collaborator

@mitchellirvin mitchellirvin left a comment

Choose a reason for hiding this comment

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

hi @khvr1993 ! thanks so much for working on all of these problems.

a couple things:

  1. do you mind removing the test files (we currently don't track test files in any other language/area, and it's highly likely they would diverge from the source code over time, though I love your enthusiasm for tests!)
  2. do you mind moving all of the solutions to the top level go/ directory? admittedly this guidance isn't clear, but I think we're going to move to keeping all solutions in the top level dir of each language to make searchability easier
  3. do you mind updating the file names to match the existing conventions? details can be found here: https://github.com/neetcode-gh/leetcode#guidelines
  4. do you mind breaking this PR up into a few smaller PRs for review? I usually wouldn't recommend doing more than a handful of solutions per-PR, as it increases the time it takes for review and merging, and it decreases the confidence we have that everything we're merging is correct

Once these things are done, I'll go through and review each solution and verify it passes submission on leetcode.com (and that there isn't already an existing solution in the repo for go). we're super grateful for the contributions, but in the future if you keep your PRs smaller, it'll take less time to get it reviewed and merged!

@khvr1993
Copy link
Author

@mitchellirvin Thanks for the suggestions.

  1. I added test files so that the users could run most of the basic code and try it out themselves. since for Go main package is mandatory to run the code, having tests would make life easy to run sample cases. I will remove them as other languages are not having them
  2. Sure
  3. Sure
  4. I will make the changes and try to make small PRs. I thought I could add solutions in bulk and merge them at once. It makes sense that the reviewer would get overwhelmed.
    All the codes are tested in leetcode and are accepted.

@mitchellirvin
Copy link
Collaborator

@khvr1993 thanks so much! Please link the PRs when they're open and I'll review them as soon as I can.

I trust that you've submitted them on leetcode and they pass, I just always have to go run them myself just in case!

@Ahmad-A0
Copy link
Collaborator

I'll close this as stale since the directory structure for go solutions has changed, feel free to update the structure in which case I'll reopen and merge this.

@Ahmad-A0 Ahmad-A0 closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants