#79 Notify user if selected object is not a building

[deevroman]

I changed the isValidData method to validateDate so that it throws an exception in case of invalid data. The error is intercepted and displayed as an alert.

I also added checking HTTP codes for requests that receive information about objects. In practice, I very often forget to specify &type=relation, so it should be useful. Especially after adding osmApiUrl

[deevroman]

@Beakerboy it seems there are no tests that mock the API yet, so I would say a slight decrease in coverage is expected due to the addition of exception catching.

[Beakerboy]

What API tests do you recommend? I have a mocking system set up to mock calls to the API and I do have tests against the building object. Could you add a test to test/building.test.js for this?

[deevroman]

I don't quite understand how you want to organize API tests. On one hand, I see that you redefined the API URL in test/src/apis.js. But the API is only used in Building.create(), and it is not covered by tests.

On the other hand I see jest-fetch-mock mentioned, but I don't see how it is used. It seems better to use the library than to make requests to the network, even though they are made to https://beakerboy.github.io/OSMBuilding/test the result of which is known for sure. I haven't used this library, so I'll need some time to figure it out.

[Beakerboy]

What I’m trying to get at is, the changes here are all within the Building class, not the API. There’s plenty of ways to get data into the Building class to make sure that an error is printed or thrown when non-building data is provided.

maybe this gets to the heart of the problem, should the Building class be making network calls at all? Should it be agnostic and just use the data that is given to it from whatever source? Maybe we need better dependency separation to do dependency injection.

[deevroman]

yeah, it would be nice to split Building.create() into something like

const data = await loadData(type, id);
new Building(id, data);

[Beakerboy]

The constructor can be tested without having to call create(), so let’s at least get a couple tests for that.

Also…in case it was not obvious, I barely know how to program JavaScript, and I only dabble in programming in general. I’m more than happy to change the way any of this is done to make it more in line with JavaScript norms, or to make it easier to test and extend. I had an idea of a program that I wanted and that did not exist, so I took it as an opportunity to learn JavaScript. Smaller, more focused pull requests are better so I can see what’s going on and ask questions.

[deevroman]

No problem, let's do it in small commits. I converted complex PRs into drafts

p.s. writing a 3D render is quite a non-trivial task :)