Skip to content

Conversation

jgeewax
Copy link
Contributor

@jgeewax jgeewax commented Jul 7, 2015

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 7, 2015
@jgeewax jgeewax added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 7, 2015
@jgeewax jgeewax force-pushed the search branch 2 times, most recently from c23dec4 to 60a2d6f Compare July 7, 2015 14:45
@dhermes
Copy link
Contributor

dhermes commented Jul 7, 2015

21 files in one commit is a mouthful! Can you start with an MVP and build up for the sake of review? The bigger the PR is, the less a chance any line actually has of being reviewed.

Also, it looks like you are using _Monkey on objects, but IIUC @tseaver only ever intended it to be used on modules.

@jgeewax
Copy link
Contributor Author

jgeewax commented Jul 7, 2015

@dhermes : Still don't merge. Want me to delete the PR until it's ready ? (_Monkey is all gone)

I get that it's a mouthful..... I can .... stop adding stuff now... but ... tearing it apart seems like a boatload of extra work just to put it back in... Any other ideas?

@jgeewax
Copy link
Contributor Author

jgeewax commented Jul 7, 2015

Maybe we can start with just the search-api.rst file...?

@dhermes
Copy link
Contributor

dhermes commented Jul 7, 2015

It is a boatload of work, but we would never let a new contributor make a PR with a brain dump, so we should hold ourselves to the same standard. The quality of review goes down and burden on reviewers goes way up when the PR is a behemoth. At the very least it should be separated into small commits encapsulating single ideas.

Start with a skeleton of each class/method and then build out features piecemeal.

If you don't have cycles for it, I can pull apart your PR to make it easy to review.

@tseaver
Copy link
Contributor

tseaver commented Oct 14, 2015

Obsoleted by #1162, #1164, #1165, #1173, #1177, #1178, #1179, #1183, #1184

@tseaver tseaver closed this Oct 14, 2015
@tseaver tseaver added the search label Oct 14, 2015
parthea pushed a commit that referenced this pull request Sep 4, 2025
parthea pushed a commit that referenced this pull request Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants