Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial commit of Cloud Search API #972

Closed
wants to merge 1 commit into from

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
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