Skip to content

Conversation

SergTanchenko
Copy link
Contributor

Hello,
Added page and per_page params
according to https://docs.github.com/en/enterprise-server@3.4/rest/search#search-repositories specification, page and per_page search params are supported

@SergTanchenko
Copy link
Contributor Author

hello @dziemba,
I am contacting you because I saw you did release recently,
could you please help me review/merge/release this PR?

* The number of results per page (max 100). Default: 30
*/
@SuppressWarnings("checkstyle:methodname")
Optional<Integer> per_page();
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to use the java method naming convention of camelCase names here and instead use the @JsonProperty("per_page") annotation to make it compatible with github's format. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to use the java method naming convention of camelCase names here and instead use the @JsonProperty("per_page") annotation to make it compatible with github's format. WDYT?

I'd like to use camelCase as well, but it turned out that serialization will be broken in this case:

Here is the method from Parameters.java:

/**
   * Goes through all public methods defined in an interface that extends this interface and calls
   * them in the context of the class that called this method, then joins the method name with the
   * result it produced using an ampersand (&amp;) as a delimiter.
   *
   * <p>It works on interfaces with deep inheritance and filters out any methods defined on this
   * interface (with the assumption that they come from the same class loader).
   *
   * @return String of "key=value" joined on &amp;
   */
  default String serialize() {
    return Arrays.stream(this.getClass().getInterfaces())
       ...
        .filter(method -> !method.getDeclaringClass().equals(Parameters.class))
        .collect(
            toMap(
                Method::getName,
                method -> {
                  ...
        .sorted((entry1, entry2) -> entry1.getKey().compareTo(entry2.getKey()))
        .map(entry -> entry.getKey() + "=" + entry.getValue().get())
        .collect(joining("&"));
  }

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying. You're right, since we're not dealing with JSON here at all my suggestion won't work. This is fine as-is then 👍

Copy link
Member

@dziemba dziemba left a comment

Choose a reason for hiding this comment

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

Thanks!

@dziemba dziemba merged commit 6d8a4d7 into spotify:master Jun 27, 2022
@dziemba
Copy link
Member

dziemba commented Jun 27, 2022

I just released this as v0.1.36, which should be available to download in a few hours.

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

Successfully merging this pull request may close these issues.

2 participants