WIP: Bring Up To Par With Geocoder 3.3.0 #49
Conversation
There was a problem hiding this comment.
Hi Mike, thanks for the great reworking of this lib. I have a few concerns regarding the choice of a new (proprietary?) CI as well as CS/CHANGELOG minor tweaks.
I must ensure the longevity of the project, that is why I tend to stick to well-known/sustainable/reliable tools and conventions.
Overall, I like the effort you put in this PR, and I don't want to restrain you. Thank you!
| @@ -1,16 +0,0 @@ | |||
| language: php | |||
There was a problem hiding this comment.
Hey, travis is known by almost everyone on GitHub/in Open Source. I'd rather keep it, because I am not sure about the new one you introduced (which I did not know), and it is always risky to use new tools for this (I like when such things are as boring as possible, so using reliable/known tool seems better to me).
There was a problem hiding this comment.
I will add TravisCI back in, not a problem. I use PHPCI because it provides so much more than just running unit test (PHPMD, PHPCS, PHPCPD, detailed code coverage reports).
There was a problem hiding this comment.
Oh, one more note on Travis ... I was hesitant to keep it, because it is failing due to a CURL bug on their end, and I didn't want to have a failing badge on the repo when it really isn't failing. I will add the travis config back, and leave the badge off for now. Once they are green, I will add the Travis badges back on.
| @@ -1,41 +1,64 @@ | |||
| CHANGELOG | |||
There was a problem hiding this comment.
One more non-coding remark: I found the previous version not readable, but I still find the new version complicated. I would use lists of short sentences (and no section).
There was a problem hiding this comment.
I would recommend you not to change everything as the existing project's layout was pretty the same as the other projects in the Geocoder org. Yet, feel free to make the changes you wish, but you're not alone ;-)
There was a problem hiding this comment.
The reason I structure CHANGELOG files this way is to adhere to the suggested conventions established at http://keepachangelog.com/en/0.3.0/. It enables not only humans to read it as a markdown document, but for systems to analyze it and work with it in a structured manner. I know there isn't a stndard, but I like to adhere to this recommendation in all my packages. If you feel this is not a good direction, I will revert this back.
There was a problem hiding this comment.
Ah, that's a good point. Let's use your structure then!
| </whitelist> | ||
| </filter> | ||
| <php> | ||
| <env name="APP_KEY" value="base64:Xgs1LQt1GdVHhD6qyYCXnyq61DE3UKqJ5k2SJc+Nw2g="/> |
There was a problem hiding this comment.
Are you sure we can keep this value here? It's laravel specific, isn't it?
There was a problem hiding this comment.
Yes, this is not a sensitive secret or password of any kind. Its only used for testing purposes. Laravel just needs a unique app-key for testing purposes.
| @@ -0,0 +1,24 @@ | |||
| <?php namespace Geocoder\Laravel\Exceptions; | |||
There was a problem hiding this comment.
could you use PHP CS Fixer with PSR 2?
There was a problem hiding this comment.
(All code is valid PSR1 and PSR2.) I have adopted moving the namespace to the same line as the PHP tag to save space. For me it makes contextual sense to have them on the same line. I realize now that the other files have the namespace below the license comment, I will stick with that, if you prefer that?
| { | ||
| $this->assertInstanceOf(ProviderAndDumperAggregator::class, app('geocoder')); | ||
| } | ||
| } |
|
Hi Will, thanks for the comments. I commented on them, and will move forward in the way you see fit (I just wanted to provide some context as to why things were done the way they were). :) Thanks! |
|
@mikebronner thanks for you work and your replies! Sounds like we are on the right path, and this should get merged soon 👍 |
|
Hey @mikebronner! I'd like to apply your PR to my local project but I could not find the |
|
Thanks for your replies, @willdurand! I will get this updated and merged this evening. Thanks for your interest, @jgrossi! :) I will make some final tweaks to the PR, then merge and publish a release candidate you can use (some time this evening PST) -- should be ready in the next 24 hours. Thanks for your patience! :) |
|
Thanks @mikebronner :-) |
Code Coverage
This PR brings the package up to par with Geocoder 3.3.x, as well as addresses bugs and adds new functionality, resolving most of the open issues.
This is a backwards-compatibility-breaking update, so it should be versioned as 0.7.0. Please read the updated README and CHANGELOG for all the details.
Currently, Travis is failing due to a CURL bug, but locally tests are all green. There are some additional tests that are missing in order to get code coverage up, but I will add them in a following PR, so as not to hold this one up.