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.

Sign up for GitHub

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

Jump to bottom

Add initial version of Review Window #1

Merged
merged 8 commits into from
Nov 10, 2015
Merged

Add initial version of Review Window #1

merged 8 commits into from
Nov 10, 2015

Conversation

Shredder121
Copy link
Member

No description provided.

timowest
timowest reviewed Sep 3, 2015
View reviewed changes
src/main/java/com/querydsl/github_review_window/GithubReviewWindow.java
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.querydsl.github_review_window;
Copy link
Member

Choose a reason for hiding this comment

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

consider using a package without underscores in the name

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you suggest?

@Shredder121 Shredder121 force-pushed the initial-version branch 9 times, most recently from d3ab921 to 5797386 Compare September 5, 2015 11:59
Shredder121
Shredder121 reviewed Sep 5, 2015
View reviewed changes
src/main/java/com/querydsl/webhooks/GithubReviewWindow.java
*
* <p />
* usage:
* {@code java [-Dduration.(labelName)=(durationString)] -jar gh-review-window-{version}-full.jar }
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you, @johnktims maybe cook up some documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, should I remove the @author line?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to put the example for running the project in a README.md?

I normally don't put the @author line in code I write, but feel free to do it. It's just a matter of preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think also putting it in README.md is fine yes, but otherwise the code is so empty of comments? What do you think?

@Shredder121
Copy link
Member Author

Do you think that configuring a server.context-pathup front in the .properties file is useful?
If it's hosted on / then there will occasionally be bots (that can't do anything useful, but still).

@johnktims
Copy link
Member

I'm not worried about it.

@Shredder121
Copy link
Member Author

Okay, then it will be up to the user.

@Shredder121
Copy link
Member Author

I added a fix that John showed me by opening a PR.
Now it is stable enough.

@timowest
Copy link
Member

Could you also add a README in this PR?

@Shredder121 Shredder121 force-pushed the initial-version branch 6 times, most recently from 8fd71db to 8fa46fa Compare October 11, 2015 19:06
@Shredder121
Copy link
Member Author

README added and remembered that John wanted a newer Spring Boot version.

@Shredder121
Copy link
Member Author

I still needed to document the actual commit status procedure.
That has been done now.

@Shredder121 Shredder121 force-pushed the initial-version branch 2 times, most recently from 958e691 to 030fa8c Compare October 12, 2015 05:22
@johnktims
Copy link
Member

Should we add an integration test and travis integration to make sure it starts up correctly?

@Shredder121
Copy link
Member Author

Yeah, that is fine, I'll do that.

@Shredder121
Copy link
Member Author

Integration test added, @timowest could you enable Travis integration?

@Shredder121 Shredder121 force-pushed the initial-version branch 3 times, most recently from 3a1d45e to 3780c08 Compare October 21, 2015 18:54
@Shredder121
Copy link
Member Author

It's not running tests, one second.

@Shredder121
Copy link
Member Author

Yeah..
Could you add a github_oauth environment variable?
Preferably with an actual oauth key, but not required as we're just asserting startup.

@Shredder121
Copy link
Member Author

Tests have finished after a workaround due to hub4j/github-api#226.

@Shredder121
Copy link
Member Author

Exactly 400 characters, nice!

@timowest
Copy link
Member

Looks good.

@johnktims
Copy link
Member

Nice work, Ruben. This looks good to me.

johnktims added a commit that referenced this pull request Nov 10, 2015
@johnktims
Merge pull request #1 from querydsl/initial-version
d7a843b 
Add initial version of Review Window
@johnktims johnktims merged commit d7a843b into master Nov 10, 2015
@johnktims johnktims deleted the initial-version branch November 10, 2015 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Shredder121 @johnktims @timowest