Apply language level migrations and clean-ups #33

jeromegamez · 2022-01-21T23:26:58Z

Hey there 👋,

as promised (threatened 😅), here's my follow-up PR. Since we can now expect to be on at least PHP 7.4 and PHPUnit 9.5, there are some improvements that can be applied.

I tried to be as atomic as possible, with each commit message describing what it is about, but I think some of the changes warrant a little more context:

Since we now have type-hints, PHPDocs are even more meant for documentation rather than support for IDEs or API Doc generation. That's the reason why I removed all property, argument and return value annotations that are already visible through the type hints.
There's no need for reflection or func_get_args() anymore \o/
According to the RFC, the "id" property of a Feature is expected to be a JSON string or a number (https://www.rfc-editor.org/rfc/rfc7946#section-3.2), so I added a check for this. This would be a breaking change, so if you're targeting a 1.1 release instead of a 2.0 release, I can remove the check again.
Testing the unhappy paths of the GeoJson::unserialize() method is not strictly part of the language level migration, but it was the only method that prevented a 100% code coverage and even if its importance can be disputed, I still think that it's a nice metric 😅
The array<...> notation in DocBlocks is used to avoid consecutive [][][]s and to make it a little bit easier to make out the supported types - especially for the nested ones. The changes should enable IDEs like PHPStorm to provide autocompletion and Static Code Analysis Tools like PHPStan to parse them.

Closes #21
Closes #22
Closes #30

jeromegamez · 2022-01-21T23:28:59Z

Speaking of release 1.1 vs 2.0 - I stumbled upon this sentence in the RFC (https://www.rfc-editor.org/rfc/rfc7946#appendix-B.1):

B.1. Normative Changes

Specification of coordinate reference systems has been removed, i.e., the "crs" member of [GJ2008] is no longer used.

The according classes could be removed for a 2.0 release, but 1.1 would have to keep them. What are your thoughts?

jeromegamez · 2022-01-22T01:05:22Z

Ugh, I realize that this is quite much. If you prefer another way of me submitting these changes or if you think that's all not needed, please let me know 🙈

jmikola · 2022-02-11T00:16:24Z

@jeromegamez: Just wanted to let you know that I haven't forgotten about this PR. I just haven't had the time to take a look at it. There's definitely no need to split this up, as I realize that'd be a pretty tedious process given the commits all touching the same files.

src/GeoJson/Geometry/Polygon.php

src/GeoJson/Feature/Feature.php

src/GeoJson/GeoJson.php

src/GeoJson/BoundingBox.php

src/GeoJson/CoordinateReferenceSystem/CoordinateReferenceSystem.php

src/GeoJson/CoordinateReferenceSystem/Linked.php

src/GeoJson/CoordinateReferenceSystem/Named.php

src/GeoJson/Exception/UnserializationException.php

src/GeoJson/Feature/Feature.php

jmikola · 2022-03-24T00:55:19Z

@jeromegamez: Thanks for all of the work on this PR, and apologies again for the delay in setting time aside to review this. I'll give you some time to address my feedback but if you don't get a chance (understandable) I will try to circle back next week, apply my own suggestions, and merge directly.

jmikola · 2022-03-24T20:19:58Z

I'll aim to take another look at this before the weekend and merge it down. Thanks very much for the quick follow-up on my late review!

jeromegamez · 2022-03-25T08:16:19Z

Thank you for wading through all the changes!

When you merge the PR, will you do it as a single , squashed commit or as a rebase with all commits? If it's the latter, I would like to clean them up first by squashing some of them (if you don't mind the force push).

jmikola · 2022-03-25T14:03:34Z

I typically do squashing but I see that I did a rebase for your previous PR. I think that was because I manually added some of my own and ended up merging via the command line.

Anyway, I'm happy to do a rebase here. That may prove more helpful down the line if we ever need to refer back to a logical change. Feel free to squash and force push as you like.

… and change http schemas with https, while we're at it

see https://www.php.net/manual/en/functions.arguments.php#functions.variable-arg-list

It should be clear that composer dependencies need to be installed in order to work on and test the library, so the `bootstrap.php` file can be deleted. Since USAGE.md has been merged into README.md, the `site` directory is not needed anymore.

jeromegamez · 2022-03-26T00:49:17Z

Alright, I think I'm done with cleaning up the commits (it was mostly integrating your suggested changes in the respective commits), and this brought us from 21 commits down to 12, including the deprecation of the CoordinateReferenceSystem which I forgot earlier.

The phpcs.xml.dist and GitHub workflow files were copied and slightly adapted.

See https://www.rfc-editor.org/rfc/rfc7946#appendix-B.1

jeromegamez · 2022-03-30T14:44:17Z

The composer docs have more information about the allow-plugins directive at https://getcomposer.org/doc/06-config.md#allow-plugins

I think the Slevomat is considered a plugin because it's type is phpcodesniffer-standard instead of library, but I'm not 100% sure.

jmikola · 2022-03-30T14:39:10Z

src/GeoJson/BoundingBox.php

-     * Constructor.
-     *
-     * @param float[] $bounds
+     * @param array<float|int> $bounds


Thanks for catching this. I see it agrees with "BoundingBox values must be integers or floats" below.

jmikola · 2022-03-30T14:39:34Z

composer.json

+    "config": {
+        "allow-plugins": {
+            "dealerdirect/phpcodesniffer-composer-installer": true
+        }


Just noticed this. Is this actually required for anything? I haven't come across this before and we don't use in mongo-php-library.

Looking at phpcs.xml.dist, I only see references to the standard rules that ship with phpcs and Slevomat, which we explicitly require above.

The composer docs have more information about the allow-plugins directive at https://getcomposer.org/doc/06-config.md#allow-plugins

I think the Slevomat is considered a plugin because it's type is phpcodesniffer-standard instead of library, but I'm not 100% sure.

Ok. I'll clone this later tonight and try to run phpcs without this plugin. If all goes well (and Slevomat rules are checked), I'll see if this can be removed. Otherwise, I'll keep it in.

Either way, I'll expect to have this PR merged Thursday. Thanks again!

Testing out the PR locally, I noticed this wasn't required to run phpcs; however, I read up further on the Composer 2.2 changes and the PHPCSStandards/composer-installer README and now understand this is a forward-thinking change to ensure Slevomat is properly installed after the new security rules take effect in July.

LGTM.

jeromegamez force-pushed the language-level-migration branch from 3a68197 to 15a53cc Compare January 21, 2022 23:35

jeromegamez added 2 commits January 22, 2022 03:31

Use ::class instead of hard-coded strings

89823ab

Use imports instead of fully qualified names

fbc0b08

jeromegamez force-pushed the language-level-migration branch from b51ebe7 to 0e0a1ec Compare January 22, 2022 02:32

jmikola mentioned this pull request Mar 24, 2022

PHP 8.1: Return type of GeoJson\GeoJson::jsonSerialize() #34

Closed

jmikola reviewed Mar 24, 2022

View reviewed changes

jeromegamez added 4 commits March 26, 2022 01:14

Simplify method mocks

394603d

… and change http schemas with https, while we're at it

Remove see annotations for native PHP functions and parent methods

a892ac0

Type-hint parameters, properties and return values

ba6c9b1

Use variable-length argument lists instead of func_get_args()

617df07

see https://www.php.net/manual/en/functions.arguments.php#functions.variable-arg-list

jeromegamez force-pushed the language-level-migration branch from f931387 to 8ed3b70 Compare March 26, 2022 00:20

jeromegamez added 2 commits March 26, 2022 01:33

Remove obsolete files and directories

9329d5c

It should be clear that composer dependencies need to be installed in order to work on and test the library, so the `bootstrap.php` file can be deleted. Since USAGE.md has been merged into README.md, the `site` directory is not needed anymore.

Test unhappy paths of GeoJson::jsonUnserialize()

b0fc034

jeromegamez force-pushed the language-level-migration branch from 8ed3b70 to edfa221 Compare March 26, 2022 00:35

jeromegamez added 4 commits March 26, 2022 01:51

Add PHPCS and use the PSR-12 coding standard

93504d3

The phpcs.xml.dist and GitHub workflow files were copied and slightly adapted.

Add .gitattributes

a31a56a

Use get_debug_type() instead of manually determining the value type

aee4cb0

Deprecate CoordinateReferenceSystem and its child classes

a2915ec

See https://www.rfc-editor.org/rfc/rfc7946#appendix-B.1

jeromegamez force-pushed the language-level-migration branch from 2cc1c1f to a2915ec Compare March 26, 2022 00:52

jmikola reviewed Mar 30, 2022

View reviewed changes

jmikola merged commit fbe590b into jmikola:master Apr 1, 2022

jmikola added this to the 1.1.0 milestone Apr 1, 2022

jeromegamez deleted the language-level-migration branch April 1, 2022 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply language level migrations and clean-ups #33

Apply language level migrations and clean-ups #33

jeromegamez commented Jan 21, 2022 •

edited

Loading

jeromegamez commented Jan 21, 2022

jeromegamez commented Jan 22, 2022

jmikola commented Feb 11, 2022

jmikola commented Mar 24, 2022

jmikola commented Mar 24, 2022

jeromegamez commented Mar 25, 2022 •

edited

Loading

jmikola commented Mar 25, 2022

jeromegamez commented Mar 26, 2022

jeromegamez commented Mar 30, 2022

jmikola Mar 30, 2022

jmikola Mar 30, 2022

jeromegamez Mar 30, 2022

jmikola Mar 30, 2022

jmikola Apr 1, 2022

Apply language level migrations and clean-ups #33

Apply language level migrations and clean-ups #33

Conversation

jeromegamez commented Jan 21, 2022 • edited Loading

jeromegamez commented Jan 21, 2022

jeromegamez commented Jan 22, 2022

jmikola commented Feb 11, 2022

jmikola commented Mar 24, 2022

jmikola commented Mar 24, 2022

jeromegamez commented Mar 25, 2022 • edited Loading

jmikola commented Mar 25, 2022

jeromegamez commented Mar 26, 2022

jeromegamez commented Mar 30, 2022

jmikola Mar 30, 2022

Choose a reason for hiding this comment

jmikola Mar 30, 2022

Choose a reason for hiding this comment

jeromegamez Mar 30, 2022

Choose a reason for hiding this comment

jmikola Mar 30, 2022

Choose a reason for hiding this comment

jmikola Apr 1, 2022

Choose a reason for hiding this comment

jeromegamez commented Jan 21, 2022 •

edited

Loading

jeromegamez commented Mar 25, 2022 •

edited

Loading