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

Fixes #183: added a method listForks() to GHRepository #185

Merged
merged 4 commits into from
Jul 17, 2015
Merged

Fixes #183: added a method listForks() to GHRepository #185

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b750be
Fixes #183: added a method listForks() to GHRepository
marc-guenther Apr 29, 2015
a5425a3
improved javadoc for listForks()
marc-guenther May 28, 2015
a83aad2
renamed Sort enum, some cleanup, better javadoc
marc-guenther Jul 17, 2015
23c56ff
remove unused import
marc-guenther Jul 17, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/main/java/org/kohsuke/github/GHRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,10 @@ public boolean isFork() {
return fork;
}

/**
* Returns the number of all forks of this repository.
* This not only counts direct forks, but also forks of forks, and so on.
*/
public int getForks() {
return forks;
}
Expand Down Expand Up @@ -505,6 +509,36 @@ public void delete() throws IOException {
}
}

/**
* Sort orders for listing forks
*/
public static enum Sort { NEWEST, OLDEST, STARGAZERS }
Copy link
Contributor

Choose a reason for hiding this comment

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

Change name to ForksSort, as it already 3 Sort classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I looked how similar stuff was implemented, so yes, I know there are 3 other Sort enums. Mine is also the only one with javadoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't copypaste all code and change only request, you should implement new feature in the most actual way. Or this lib became unmaintainable soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everyone adding code in their own particular coding style also makes this library hard to read and understand. Also, this is not some cool new feature that needed to be implemented, I just added one missing api method (out of, I don't know, a hundred?), that for some reason has been overlooked in the past.

Implementing this in a "new way" by someone like me who doesn't know this library at all is way more error prone, as I don't know the particulars of the currently chosen approach.

Also now, if someone wants to clean up this code, at least everything looks the same, which imho makes cleaning up much easier than if everything looks different.


/**
* Lists all the direct forks of this repository, sorted by {@link Sort#NEWEST Sort.NEWEST}
*/
public PagedIterable<GHRepository> listForks() {
return listForks(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

why just not to pass default value and not handle null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I don't know the default. It is defined by github, and they do not expose it, except in their documentation.
So my javadoc is wrong, it should read "sorted by whatever github api defined as the default sort order, currently NEWEST"

}

/**
* Lists all the direct forks of this repository, sorted by the given sort order.
* @param sort the sort order. If null, defaults to {@link Sort#NEWEST Sort.NEWEST}.
*/
public PagedIterable<GHRepository> listForks(final Sort sort) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add @CheckForNull and describe the behavior in Javadoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not a single @CheckForNull in the entire codebase so far. If you want to start introducing them, go ahead, but that was not the scope of this pull request.
Regarding Javadoc, I was under the impression that both methods I added actually have Javadoc comments, but maybe I miscounted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My intention was to highlight the method accepts nulls and to describe the behavior in that case.

There is not a single @checkfornull in the entire codebase so far. If you want to start introducing them, go ahead, but that was not the scope of this pull request.

I think that new pull requests could follow better practices than the legacy code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Just added (hopefully) better Javadoc. Unfortunately, I was not able to find out how to add the @CheckForNull annotation.

return new PagedIterable<GHRepository>() {
public PagedIterator<GHRepository> iterator() {
return new PagedIterator<GHRepository>(root.retrieve().asIterator(getApiTailUrl("forks" + ((sort == null)?"":("?sort="+sort.toString().toLowerCase(Locale.ENGLISH)))), GHRepository[].class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(sort == null)?"":("?sort="+sort.toString().toLowerCase(Locale.ENGLISH))))
this line is too difficult to understand - can you use separate method for this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is quite easy, if a sort is given, use it, otherwise don't.

Sorry, but I don't know how to write this more clearly...

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Spaces. Use them.
public PagedIterator<GHRepository> iterator() {       
               return new PagedIterator<GHRepository>(root.retrieve()
                              // statically imported org.apache.commons.lang.ObjectUtils.defaultIfNull
                               .with("sort", defaultIfNull(sort, Sort.NEWEST).name().toLowerCase())
                               .asIterator(getApiTailUrl("forks")), GHRepository[].class)) {

Actually it is quite easy, if a sort is given, use it, otherwise don't.

If one line gets more than 10s to understand code - it need to be refactored

@Override
protected void wrapUp(GHRepository[] page) {
for (GHRepository c : page)
Copy link
Contributor

Choose a reason for hiding this comment

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

please use brackets if there only one line inside too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been copied verbatim from other list* method, and it is like this all over the place in this file.

Is this really a reason to not merge this pull request?

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been copied verbatim from other list* method, and it is like this all over the place in this file.

Don't follow bad practises :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, different people have quite different opinions on these kind of things, and as I don't know this particular project's conventions, I decided to stick with what I found in the existing source...
I also didn't think this is such a big deal...

Copy link
Contributor

Choose a reason for hiding this comment

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

oracle guide is main document on how to format code. If no rules in project, oracle guide should be used

+ "if" without brackets its a point where you can miss something - because of no visual edge of code block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is your opinion, obviously not that of the original author.

I find it more important to follow existing conventions when making such a small contribution to an existing bigger project, rather than to impose my current opinion about what is good or bad coding style onto it. I consider consistency to be more important than personal taste. (and I completely agree with you that you always should use brackets, but that is besides the point)

I also smell some serious case of bikeshedding here...

Copy link
Contributor

Choose a reason for hiding this comment

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

Lot of minor issues make library unstable, untestable and difficult to read and hard to improve

c.wrap(root);
}
};
}
};
}

/**
* Forks this repository as your repository.
*
Expand Down