I noticed there was a maps field not detailed in the API.

[BrentonPoke]

The added lines are

 private ArrayList maps;

 public ArrayList getMaps() {
        return maps;
    }

    public void setMaps(ArrayList maps) {
        this.maps = maps;
    }

starting at line 35 I had to destroy my other fork (again) so it's not highlighting them. Anyway, some tournaments are going to list the map pool, and that's communicated in an array as

{
maps: [ map1, map2, map3, ... mapN ]
}

This wasn't outlined as a possibility in the toornament api documentation, and I found out about it in debugging.

[BrentonPoke]

I have another addition right after this pull request. It implements returning results from v1/tournaments using a list of parameters.

[skamoen]

Can you try to fix your git setup so that we don't have this file-replace issue anymore? This probably won't be the last time this happens and it makes reviewing a lot easier if I can see the actual changes.

It probably has to do something with your git settings regarding line endings. This might help: https://help.github.com/articles/dealing-with-line-endings/

[BrentonPoke]

I changed the line endings to CRLF, but i'm not sure if that solves it.

[BrentonPoke]

I don't think the line endings were ever an issue, because they were the same as the upstream's. I'm not sure why git doesn't do a diff on all the files for each commit.

As for the code, this is what was added for parameters:

import javafx.util.Pair;

public List<Tournament> getTournamentsWithParams(List<Pair<String,String>> paramsList ) {
        String url = "https://api.toornament.com/v1/tournaments?";

        for (Pair<String, String> params : paramsList) {
            url += params.getKey() + "=" + params.getValue() + "&";
        }

        System.out.println(url);
        Request request = client.getRequestBuilder()
            .get()
            .url(url)
            .build();
        try {
            String responseBody = client.executeRequest(request).body().string();
            return mapper.readValue(responseBody, mapper.getTypeFactory().constructCollectionType(List.class,
                Tournament.class));
        } catch (IOException e) {
            e.printStackTrace();
            throw new ToornamentException("Couldn't retrieve tournaments");
        }

    }

[skamoen]

This behaviour is pretty typical for wrong line endings. Note that changing settings doesn't affect commits already made.

Maybe try re-installing Git on your machine and leave the default option in the line options menu? It's very hard to see what's wrong from here, but please try fixing it. This seems related: https://stackoverflow.com/questions/1889559/git-diff-to-ignore-m

[BrentonPoke]

I won't know until another commit is made, but I can change it back to CRLF.

Sent from my Verizon, Samsung Galaxy smartphone null

[skamoen]

The diff is correct when using git diff -b

-b --ignore-space-change Ignore changes in amount of whitespace. This ignores whitespace at line end, and considers all other sequences of one or more whitespace characters to be equivalent.

[BrentonPoke]

When you say "the diff is correct", what exactly do you mean? The diff of the edits I committed?

[skamoen]

When I manually run git diff -b on your changes, I get the correct changes.

(To do this, I added your repo as a remote, so git diff -b biokinetica/master checks for differences between the repo's)

You can do the same thing with your repo:

git remote add upstream git@github.com:AreaFiftyLAN/toornament-client.git

and then git diff -b upstream/master

[BrentonPoke]

I don't get why git doesn't run that on my end automatically. null

[BrentonPoke]

So is there no way to "commit" the same stuff that's already committed in a way that github recognizes the changes? If not, I think I've done all I can or probably should in regards to the line endings.

Either way, does the code look good?

[skamoen]

It's not about github, it's about the underlying git mechanics. As this a relatively simple PR, fixing it now will save so much more trouble in the future when PR's become more complicated. It's important for Github workflow that ths is fixed.

Try this:

git config --global core.autocrlf true

git remote add upstream git@github.com:AreaFiftyLAN/toornament-client.git

git reset --hard upstream/master This will reset all changes you made. Re-add the code to the file (as you posted in the PR description)

git commit -am "Added maps to TournamentDetails" After this your changes should be committed with the correct line endings. You can check with

git diff upstream/master If that shows the line changes instead of the entire file, awesome, we're done.

If not, check git diff -b upstream/master. If that does show the correct changes, it's still not fixed.

[BrentonPoke]

I've tried this already and just get this after trying to reset.

[skamoen]

try git fetch upstream first, then run git reset --hard upstream/master again

[BrentonPoke]

I'm not sure what this is supposed to be accomplishing, but now I can't commit anything to my own branches.

[skamoen]

I really want to help you fix this issue, but as our environments are obviously different, I have no idea what errors you might be having. Please me more descriptive about the issues you're having.

[BrentonPoke]

I got the working branch reset and changes recommitted, but now I can't get my changes from the working branch to this branch that the pull request is coming from.

[skamoen]

It seems you're undoing your recommit with the merge in this commit: https://github.com/Biokinetica/toornament-client/commit/19586631ca3b13aa2d85b9d6deab38a06e420b69

The git reset is to start off clean, only add new commits to that branch, don't merge other branches back in again

[BrentonPoke]

Start off clean with which branch? The working-branch is for saving test runs. I should be able to just commit the working code to the master branch and do pull requests from that branch. Right now, the pull request doesn't even have my changes in the latest commit.

[skamoen]

Can I recommend the feature branch workflow for contributions like this? You make a PR from a branch containing only the changes for that feature and you keep your master branch synced with my master branch. That prevents a lot of merging, reverting and other messy git operations.

That way you can also continue working while you're waiting for a review and such.

To do this:

git checkout -b "feature-maps_field_tournamentdetails"

make sure you're in sync with my master branch: git reset --hard upstream/master

add your code for the PR

git commit -am "Added maps field to TournamentDetails"

git push origin feature-maps_field_tournamentdetails

And then create a PR from your feature branch into my master branch.

When I merge your PR, you can update your master branch with a git pull upstream master and apply the changes to your other branches with a git rebase origin/master

You can also open two PR's, for example the parameter get. Just create a feature branch for that specific change and open a new PR for only those changes.

Merge commits are messy and don't show a proper changelog. It may seem like a lot of effort, but it's quite common for open source project contributions and it greatly improves clarity and overview.

[BrentonPoke]

I'll try this, then. I got the changes back into this pull request, so I think it can be safely merged with the upstream now.

[skamoen]

I looked into the maps field. It's actually documented if you select a discipline in the top right corner at the documentation field (https://developer.toornament.com/doc/tournaments/overwatch)

That field is actually covered by the disciplineFields, which takes any fields not listed and puts them in a HashMap. This is a <String, String> hashmap, which is why you got an error. I changed it to <String, Object[]> so all fields are now stored as an array of objects. For maps, it results in an array of Strings as expected, and for any other fields that a Tournament may have that's not an array, it'll be an array with a single value.

I believe this is a better solution, as not all Disciplines have the maps field and we can't really account for all possible fields.

Can you create a separate PR for the parameter changes?

[BrentonPoke]

Ok, I'll do that.

Sent from my Verizon, Samsung Galaxy smartphone