@Guardiola31337, thanks for your PR! By analyzing the annotation information on this pull request, we identified @cammace and @zugaldia to be potential reviewers

@Guardiola31337 Thank you so much for your PR, this looks great. PRs with tests are the best PRs.

@ivovandongen Does this cover everything you had in mind for #173?

@zugaldia You're welcome!
Keep up the good work guys!

@Guardiola31337 Thanks for adding this! The only thing that could make it better is to use some fixture files instead of the inline json strings. Otherwise, it's looking 👍

@ivovandongen You're welcome!

I imagine you are talking about the constants defined in BaseGeoJSON.

I did it intentionally. In my opinion, using DAMP (Descriptive And Maintainable Procedures) you increase maintainability and readability of the tests. Let me explain why I prefer to expect a literal here.

• The main advantage of expecting literals is that you only need to focus on the actual value of the assertion, increasing readability and traceability.
  • If neither your expected nor actual values are literals you’re forced to determine the value of both when a test fails.
  • If literals are used as expected values the tests will directly reflect some or all of the examples from the business.

In addition, I think is better to have all variables defined in test classes (where they are going to be used) because it increases maintainability. When you do so, you don't have to jump into the constants file when the requirements change, decreasing the possibilities of forgetting to do it there.

When the language doesn’t provide a literal for the actual value I try to convert the value object into a literal that approximates the value as closely as possible. That was the purpose of defining obtainLiteralCoordinatesFrom methods, as you may have already noticed.

That said, what do you think?

@Guardiola31337

I imagine you are talking about the constants defined in BaseGeoJSON.

Actually, no. I was suggesting using fixture files for json test fixtures. Sorry that wasn't clear. The BaseGeoJSON doesn't really make anything more readable. (See an example of fixtures in mapbox-gl-native)

About using literals in tests for assertions; definitely, in case of simple literals. In case of multi-line strings, especially with formatting characters as well, I think that having them in well readable external files makes the purpose of the test much more clear. You can edit the fixtures with syntax highlighting and proper syntax validation as well. Also, when the tests do fail, you'll still have the 'raw' json in the test results, so everything should be as clear then.

Since the current setup (with BaseGeoJSON) doesn't really reflect our intention, this doesn't have to block this PR. We should, however, make a new ticket to clean all the json string literals up going forward.

@ivovandongen Makes sense, thanks for the 👀 . I'll open a follow up ticket.

@Guardiola31337 Thank you again for the contribution, merging now.

@ivovandongen Cool! Got it, it makes total sense 👍
I'll take a look at the issue in order to improve that.
My pleasure!