Collection

[craineum]

This should be everything for the collection request, including documentation.

[jemiahlee]

Ok, thanks for splitting these up! I'll take a look hopefully tonight, but it may take me until the weekend to get to it. End of the quarter at work, so I have features that I need to push by Monday. :-)

[jemiahlee]

no apostrophe in this its.

[jemiahlee]

Actually, I'm gonna go through the documentation myself anyway, so don't sweat the docs right now. I'm also an attorney, so I'm big on being specific about language. :-)

One thing that I want to at least talk about is the fact that you've embedded the XML in the testing structure, which makes it very difficult to verify that we are parsing the correct data. With external files that have been generated by hitting the endpoint, we can write a separate test (though I hadn't yet done so) to validate that all of the output from the BGG API hasn't changed format (and frankly, it's not a super consistent API, so the possibility of that getting effed up seems relatively high to me). What are your thoughts on that?

[craineum]

I am absolutely horrible with english, which is sad since it is my native language. So I will not take it hard if you want to completely change things.

I agree that the BGG XML API is not consistent (date has been handle three different ways so far... grrr). The reason I pulled the xml files out, and put them into the tests is for a few reasons:

1. It was confusing to flip between the included files and the test to figure things out. This allows for more isolation without confusion (in my mind at least).
2. The files were huge and greatly slowed down the tests. While stress tests are great, I don't think unit testing is the place for it.
3. I doubt the BGG API will change since they do seem to be doing versioning so far.
4. If the BGG XML API does change then we will have to change the code anyways.

Look at it this way, our code is written to certain expectations, if those expectations change then the code has to change. The unit tests are testing those expectations, so those would also need to change.

I like your idea of testing the BGG XML API, but to make that worthwhile we would need to be running that on a continual situation. To me that is a separate app to test our expectations.

with my two cents said, I am open to discussing it further ;)

[jemiahlee]

Well, I wouldn't call any of this stress testing; more like "we handle the fact that the XML returned from the API isn't pretty." I would be way more comfortable if we had basic validation that the API works on real data, and then testing the fields from the embedded XML as you've done.

[craineum]

So more of an integration test. I could generate a full set of data (maxed out fields and request) that has a couple of records. Then verify all the calls do not contain nil. This should test everything without having to worry about what is being matched, as that is being handled by the unit tests.

This way we could always generate a new set of data off the API and verify none of our expectations has changed.

That sound good?

[jemiahlee]

Yeah, I think that would be fine.

[craineum]

Ok, merged in master and added sample data for collection.

If this is good and you merge, will then go and merge into my other pull request for "search" as I am sure there will be merge conflicts.

[jemiahlee]

Ok, apologies for the delay. Looking at this right now.

[jemiahlee]

Ok, so the only thing that's weird to me is that you have a merge commit in this PR. There really shouldn't be merge commits. The PRs should be the only thing creating merge commits. Github doesn't show this as having made any modifications for the merge commit, so I'll merge it, but please use rebase in the future. That should minimize the codebase turbulence and also make it easier for you to combine your new commits.

[craineum]

I will have to learn how to use rebase for that :) This is my first time contributing to a forked item. Have always worked in primary repository. Thanks!

[jemiahlee]

Yeah, 99.9999% of the time, you want to use rebase.

[jemiahlee]

Also, my comment there really even applies for when you're working on a single repository that isn't a fork. rebase is always the better operation (because it never rewrites/changes the old sequence of commits in any way).

[craineum]

I almost always do a git pull --rebase, which I think is the same?

[jemiahlee]

Well, git pull is a compound command, consisting of a fetch and then a merge (the default) or a rebase (the way you've used it there). So yeah, that should basically always be ok, as long as the branch name is the same on origin as the one you're on.