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

Issue-179 Implement last-modified API #180

Merged
merged 2 commits into from
Mar 16, 2019
Merged

Conversation

adam-tylr
Copy link
Contributor

Note the API documentation reads as if the "at" query parameter is optional. It says it will use the default branch. However, when writing integration tests, I always got an com.atlassian.bitbucket.validation.ArgumentValidationException when I excluded it saying "Streaming modifications requires a starting commit." so I made it required.

@cdancy cdancy self-assigned this Mar 16, 2019
final Commit latestCommit,
final List<Error> errors) {
return new AutoValue_LastModifiedSummary(BitbucketUtils.nullToEmpty(errors),
files,
Copy link
Owner

Choose a reason for hiding this comment

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

Lets all use BitbucketUtils.nullToEmpty(files) here as well.

import java.util.Map;

@AutoValue
public abstract class LastModifiedSummary implements ErrorsHolder {
Copy link
Owner

Choose a reason for hiding this comment

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

Lets change the name to just LastModified to match the endpoint being used. The name here isn't horrible, it's actually fine, I just like to try and match the returning object with the endpoint name if possible.

@cdancy cdancy added this to the v2.5.1 milestone Mar 16, 2019
@cdancy
Copy link
Owner

cdancy commented Mar 16, 2019

@adam-tylr couple minor nitpicky things to address but all-in-all the code is clean and there is not much to pick at. Good job and thanks.

@adam-tylr
Copy link
Contributor Author

@cdancy I agree with both of those. I even had the nulltoEmpty(files) then talked myself out of it.

@cdancy
Copy link
Owner

cdancy commented Mar 16, 2019

@adam-tylr everything looks good on my end and thanks for the PR! I'm about to step out for a few hours but if you're looking for a point release with this change I can knock that out when I come back. Let me know.

@cdancy cdancy merged commit 62c2658 into cdancy:master Mar 16, 2019
@cdancy
Copy link
Owner

cdancy commented Mar 16, 2019

I just merged this PR but noticed there were a few more things I wanted to tackle. I can do them when I get back or if you're up for it have it but here they are:

  1. The fallback name should be changed to reflect the name changes above.
  2. Change the endpoint/method-names from listLastModified to just lastModified

@adam-tylr adam-tylr deleted the pr/179 branch March 16, 2019 16:07
@adam-tylr
Copy link
Contributor Author

I'll knock those out real quick. You don't need to create a release yet. I noticed the list-files api is only half implemented. You can't specify a path to list them at. I might tackle that this weekend if I have time.

@cdancy
Copy link
Owner

cdancy commented Mar 17, 2019

Another thought: I think i could probably write an HttpRequestFilter for this method that would allow us to combine both methods into a single version and if the path param is null we just scrub it from the request line. Thoughts?

@adam-tylr
Copy link
Contributor Author

I think that makes sense. Especially since there are several endpoints all set up the same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants