Support short names #107

Firesphere · 2018-02-10T02:20:00Z

Adding support for short classnames and updating the docs accordingly.
This works via a config, that gets the short classname to prevent IDE's from mentioning "Unnecessary FQN"

Included is a fix for a whitespace trim introduced in rc-1
Resolves #108 , SQN instead of FQN

flamerohr · 2018-02-10T02:35:32Z

src/DataObjectAnnotator.php

+            // Because the tests re-instantiate the class every time
+            // We need to make it a unique array
+            // Also, it's not a bad practice, making sure the array is unique
+            self::$extension_classes = array_unique(self::$extension_classes);


perhaps move the array_unique() to the setter and use the setter here instead, helps keep expected data more consistent.

Although valid point, moving it to the setter is not ideal, because it would call the unique method for every class that is attempted to add...

What about a in_array() check? It would result in the same outcome, but the addition not being done?

yup, that's fine too

flamerohr · 2018-02-10T02:41:05Z

src/Generators/AbstractTagGenerator.php

+                $mixinName = "\\$fieldName";
+                if (Config::inst()->get(DataObjectAnnotator::class, 'use_short_name')) {
+                    $mixinName = ClassInfo::shortName($fieldName);
+                }


could this be replaced with getAnnotationClassName($mixinName);?

Yep, missed that one :D it was a refactor after-thought, it's "implement idea" and then "think about idea", as per my usual train of thought :D

zanderwar · 2018-02-10T05:37:25Z

src/Generators/AbstractTagGenerator.php

+     */
+    protected function getAnnotationClassName($class)
+    {
+        if (Config::inst()->get(DataObjectAnnotator::class, 'use_short_name')) {


You can just DataObjectAnnotator::config()->get('use_short_name'); now - although that may just be preference but you do save 9 characters each time! :)

Good point :) Yeps, I'll need to adjust that :)

Firesphere · 2018-04-25T03:41:17Z

I think this PR is ready now.

lekoala · 2018-05-24T11:46:21Z

i hope this get merged soon, the whitespace issue is really annoying :-)

Firesphere · 2018-05-24T23:36:57Z

Ping @robbieaverill @flamerohr

Will rebase asap

robbieaverill

Looks great overall! I've left a couple of phpdoc related comments to fix up and some unrelated side comments for future consideration

robbieaverill · 2018-05-25T01:03:18Z

composer.json

@@ -29,6 +29,7 @@
  "require-dev": {
    "scriptfusion/phpunit-immediate-exception-printer": "^1",
    "phpunit/phpunit": "^5.7",
+    "silverstripe/recipe-cms": "~1",


It's better to require the CMS module than the recipe for it

This is specifically for require-dev, basically, get the Page class stuff installed in Travis

Rather not have this fixed at this point, could look in to it at a later point. Travis seems to not work the same way I work around this "issue" as CircleCI does

robbieaverill · 2018-05-25T01:04:47Z

src/DataObjectAnnotator.php

     */
    public function __construct()
    {
        // Don't instantiate anything if annotations are not enabled.
        if (static::config()->get('enabled') === true && Director::isDev()) {
+            $this->extend('beforeDataObjectAnnotator', $this);


You don't need to pass $this - it can be accessed as $this->owner from extension instances

robbieaverill · 2018-05-25T01:05:14Z

src/DataObjectAnnotator.php

+
+    /**
+     * Add another extension class
+     * @param $extension_class


Missing the type - string?

robbieaverill · 2018-05-25T01:05:54Z

src/DataObjectAnnotator.php

-            if ($pos !== false) {
+            if (class_exists($className)) {
+                $exploded = explode("\\", $className);
+                $classNameNew = end($exploded);


You could use ClassInfo::shortName($className) to do this

robbieaverill · 2018-05-25T01:06:18Z

src/DataObjectAnnotator.php

+    /**
+     * Named `setup` to not clash with the actual setter
+     *
+     * @param $extension_classes


There's no argument for this annotation

robbieaverill · 2018-05-25T01:07:10Z

src/Extensions/Annotatable.php

@@ -77,9 +82,19 @@ public function setUp()
    /**
     * @param $message
     */
-    public function displayMessage($message)
+    public function displayMessage($message, $heading = false, $end = false)


PHPDoc needs updating, is also missing a type for $message

Hah, yeah. Since I worked outside the "working directory" all the automatic PHPStorm annotations are missing :/

robbieaverill · 2018-05-25T01:09:05Z

src/Generators/OrmTagGenerator.php

+     */
+    protected function generateManyManyTags()
+    {
+        $this->generateTagsForDataLists($this->getClassConfig('many_many'), ManyManyList::class);


Does this method need to be updated, or a new one added, to support ManyManyThroughList?

That's from the rebased version, right? The generateTagsForDataLists method takes care of the optional through

robbieaverill · 2018-05-25T01:10:15Z

tests/mock/AnnotatorPageTest.php

+// @todo get it to work properly. Seems something is wrong with the installation of the CMS
+// We do need the CMS and the pages for proper testing
+if (!class_exists('\\Page')) {
+    require_once(BASE_PATH . '/mysite/code/Page.php');


We still need to get this of this code - in SS 4.2 this is in /app/src/Page.php by default, but we should be able to rely on the PSR-0 autoloader definition. Unrelated to this PR though.

Also unrelated, but require_once is a construct rather than a function so doesn't need brackets

I know it doesn't require brackets, and I agree it needs to go out.
But both are not high prio for me, unless actively developing on this module, it would not hurt anyone (yet).

The require_once not needing brackets is true, I guess it's a personal preference to use the brackets, for me that is. It just feels... "more right"

robbieaverill · 2018-05-25T01:12:31Z

tests/unit/DataObjectAnnotatorTest.php

-            AnnotatorPageTest::class                => Director::baseFolder() . DIRECTORY_SEPARATOR . 'ideannotator' . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR . 'mock' . DIRECTORY_SEPARATOR . 'AnnotatorPageTest.php',
-            AnnotatorPageTestController::class      => Director::baseFolder() . DIRECTORY_SEPARATOR . 'ideannotator' . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR . 'mock' . DIRECTORY_SEPARATOR . 'AnnotatorPageTest.php',
-            TeamSupporter::class                    => Director::baseFolder() . DIRECTORY_SEPARATOR . 'ideannotator' . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR . 'mock' . DIRECTORY_SEPARATOR . 'DataObjectAnnotatorTest_TeamSupporter.php',
+            Team::class                          => Director::baseFolder() . DIRECTORY_SEPARATOR . 'ideannotator' . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR . 'mock' . DIRECTORY_SEPARATOR . 'DataObjectAnnotatorTest_Team.php',


There is an opportunity here to re-use and shorten the line length by using some variables (unrelated to this PR though)

Also looks like this would be an area that needs updating for the module to be made into a silverstripe-vendormodule

It works for now, I agree it would be nice to clear out properly

The testing runs on the base of the module not living in the vendor folder. Which is okay, as the tests produce the same output, just with a different path. Very low priority for me

robbieaverill · 2018-05-25T01:13:44Z

tests/unit/DataObjectAnnotatorTest.php

+    public function testSetExtensionClasses()
+    {
+        $classes = DataObjectAnnotator::getExtensionClasses();
+        unset($classes[20]);


Magic numbers! 😆

I know, could not find a proper way around it when doing the initial work and left it to that :D

Fix low-prio bug with too much trimming Fix #68 simplify config calls

Firesphere · 2018-05-27T02:05:03Z

@robbieaverill @flamerohr @lekoala I think I've covered all major painpoints in this PR now. Rather have this in sooner than later, and address the leftovers in a separate PR.

The pain points this PR fixes are more important than being extremely picky about what is and isn't right, in my opinion.
The left-over pain points are also only a problem if the user is actively developing on this, instead of being a user.

flamerohr · 2018-05-27T02:13:03Z

@Firesphere I suggest an issue to list the remaining painpoints and get it in, things look fine as is :)

I'm not a big fan of leavign @TODO as they have a tendency to linger forever, but that's also an option.

Firesphere · 2018-05-27T02:27:31Z

@flamerohr I use // @todo actively in my PHPStorm and VSCode configuration, to show me where todo's are in case I have time to properly refactor things.

I agree it's not the best of practices, but it works very well for me, so I'd rather leave them in.

robbieaverill

LGTM

robbieaverill · 2018-05-27T22:02:33Z

.travis.yml

@@ -23,11 +23,12 @@ before_script:
  - composer install --prefer-dist
  - composer require --prefer-dist --no-update silverstripe/recipe-core:1.0.x-dev
  - composer require --prefer-dist --no-update squizlabs/php_codesniffer:*
+  - vendor/bin/sake dev/build flush=all


This isn't required

robbieaverill · 2018-05-27T22:05:54Z

src/DataObjectAnnotator.php

@@ -168,18 +261,18 @@ protected function writeFileContent($className)
        $filePath = $classInfo->getClassFilePath();

        if (!is_writable($filePath)) {
+            // Unsure how to test this properly


Ah the issue of direct output in build tasks (and related operations) 😆 you can test this in one of two ways:

Buffer the output of the test

Move the call to DB::alterati... into a class method, then partially mock it

Related: silverstripe/silverstripe-framework#6524

It's the make-path-unwritable-mid-operation is what I'm referring to ;)

robbieaverill · 2018-05-27T22:08:22Z

Any further thoughts @flamerohr @zanderwar ?

lekoala · 2018-07-05T07:50:31Z

any idea when this will be merged? :-) it's really annoying to have the white line removed

Firesphere requested review from unclecheese and robbieaverill February 10, 2018 02:20

Firesphere mentioned this pull request Feb 10, 2018

Namespaces are included the class name #101

Closed

Firesphere force-pushed the support_short_names branch 2 times, most recently from 1efe2ce to 57aca0c Compare February 10, 2018 02:59

Firesphere mentioned this pull request Feb 10, 2018

Field Names that look alike are annotated wrong? #68

Closed

flamerohr reviewed Feb 10, 2018

View reviewed changes

Firesphere force-pushed the support_short_names branch 2 times, most recently from cb4ae25 to 7310c36 Compare February 10, 2018 03:56

zanderwar reviewed Feb 10, 2018

View reviewed changes

Firesphere force-pushed the support_short_names branch 3 times, most recently from 88b9c33 to 7fc689a Compare February 11, 2018 02:33

zanderwar approved these changes Feb 11, 2018

View reviewed changes

Firesphere force-pushed the support_short_names branch from 16d63f4 to ad36665 Compare February 11, 2018 03:43

Firesphere mentioned this pull request Feb 11, 2018

3.0-RC1: Controller changes "Page" mixin on second run #104

Closed

Firesphere force-pushed the support_short_names branch 4 times, most recently from b7f2f64 to 85899b8 Compare February 11, 2018 05:41

Firesphere force-pushed the support_short_names branch 8 times, most recently from 5391415 to c855dbe Compare April 25, 2018 03:30

robbieaverill reviewed May 25, 2018

View reviewed changes

Firesphere force-pushed the support_short_names branch from c855dbe to 443b7f6 Compare May 27, 2018 00:36

Support for short names via config

b34dcfb

Fix low-prio bug with too much trimming Fix #68 simplify config calls

Firesphere force-pushed the support_short_names branch from a76175a to b34dcfb Compare May 27, 2018 01:55

Firesphere mentioned this pull request May 27, 2018

keep empty lines #108

Closed

This was referenced May 27, 2018

Unit tests include hardcoded paths #102

Closed

RFF: Add an extend-method to have external modules hook into annotations. #69

Closed

do not remove empty lines #109

Closed

Invalid method check for Page_Controller->data() #77

Closed

Firesphere added bug enhancement dev-issue Issue only affecting active developers labels May 27, 2018

Minor bugfixes and annotations fixes.

6ac87bf

robbieaverill approved these changes May 27, 2018

View reviewed changes

robbieaverill merged commit b4a79c8 into master Jul 5, 2018

robbieaverill deleted the support_short_names branch July 5, 2018 10:19

Support short names #107

Support short names #107

Conversation

Firesphere commented Feb 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Firesphere commented Apr 25, 2018

lekoala commented May 24, 2018

Firesphere commented May 24, 2018

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Firesphere commented May 27, 2018

flamerohr commented May 27, 2018

Firesphere commented May 27, 2018

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill commented May 27, 2018

lekoala commented Jul 5, 2018

Firesphere commented Feb 10, 2018 •

edited