#476 crud improvements #477

sallaberry · 2019-08-09T01:06:17Z

Trying to make generated code compatible with Magento type hint checks

…aces, fix empty lines missing/unnecessary

…ma in _init argument list

…iline implements

…mplate + line > 120 characters install / upgrade methods + phpdocs for repository interface methods

astorm · 2019-08-09T15:36:46Z

@sallaberry FYI -- I just opened a branch/pr where I'm going to try and get phpcs to do it's thing in our CI/CD build: #479

Once I get that running, we can incorporate that over here and ensure whatever files you intend to have pass the phpcs sniffs pass before checking stuff in.

I'll update this PR when I've got #479 working

sallaberry · 2019-08-09T17:24:27Z

Ok thank you.

Also after reading more in the already opened similar issues like #273 I realise some of the modifications I submitted might not be to your liking:
I added PHPDoc for all methods, class, interfaces, and "pretty print" _construct/_init params in multiline because of php-cs line > 120 characters... It produces a lot more "noise" but makes my IDE happier.

I agree with the noise problem, but that would be a nice feature to have maybe with an optional argument like --with-phpdocs for all commands ?

astorm · 2019-08-10T20:28:44Z

@sallaberry Re:

Also after reading more in the already opened similar issues like #273 I realise some of the modifications I submitted might not be to your liking:

That may have been true then, but this is now.

I'm not doing active, on the ground, Magento work these days (I mean, if someone threw a big giant pile of money or incentive at me I would, but that's another story for another time). I maintain pestle for the benefit of the community, and because I enjoy the privilege of having an open source project that has active users, but not so many active users that it drives me crazy :)

At this point, when it comes to the sort of code pestle generates for Magento, I'd rather leave it to someone like yourself and others who are actively engaged in working with the platform.

Re: Next steps.

I just merged a PR that adds a phpcs lint check to travis using the magento/magento-coding-standard rules. It's my understanding these are the new unified rules Magento developers are trying to adhere to.

Could you

Pull the new master into this PR
Add pestle.phar magento2:generate:... calls to for-lint.sh for the commands whose output you're changing.
Make sure whatever changes you're making apply to these standards keeps travis happy
Let me know if all this sounds reasonable, or if you have additional suggestions for improvements.
Let me know if you run into trouble making the new phpcs work.

Does that sound OK/reasonable?

sallaberry · 2019-08-10T21:56:34Z

Sounds OK.

I have more improvements in mind, but first I'll try to make theses work / pass the new integrated phpcs

sallaberry · 2019-08-10T22:33:11Z

~~Are we sure Travis is git cloning the right branch? Feels like phpcs reports warnings that I already fixed in the current branch.. Maybe this?~~

In .travis.yml :

# download latest pestle phar and install
curl -LO "$PULSESTORM_PESTLE_URL/pestle.phar"

Shouldn't it build pestle.phar from current branch instead of downloading from your site?

I'm sorry I'm not familiar with Travis that much...

astorm · 2019-08-11T00:12:36Z

Shouldn't it build pestle.phar from current branch

You're right. I'd forgotten where that pestle.phar came from and assumed it was the current branch/version. Give me 10/15 minutes and I'll have a "pestle_dev" command available for use in travis.

astorm · 2019-08-11T00:38:33Z

Going to need to adjust that 10 - 15 minutes to "later tonight or tomorrow", but I've got a dirty version working here: https://github.com/astorm/pestle/pull/480/files if you want to give it a try

This will build and install a pestle.phar from the current branch -- not that pestle_dev approach I started with.

sallaberry · 2019-08-11T00:44:29Z

Ok thanks I'll have a look.

astorm · 2019-08-11T02:54:36Z

OK, master should have now have a pestle.phar that's built off of the branch that's been PR'd

… of file

astorm · 2019-08-13T15:07:29Z

@sallaberry How are you feeling about this? Ready for me to take a look and merge? Or are you still working on stuff?

sallaberry · 2019-08-13T15:10:17Z

Yes I think it's ready if you take a look at what I've done so far. Please check I didn't break anything... I didn't modify the generated php too much so I think It's all good.

astorm

@sallaberry Thanks again for taking this on. I think we're almost there. I just submitted a review with a few comments/questions. If you can answer those and bring the branch up to date with master I think we'll be all set to merge this.

astorm · 2019-08-14T15:25:45Z

travis/for-lint.sh

@@ -11,4 +11,6 @@
 mkdir -p app/code/Pulsestorm/Travis
 printf "<?php\nnamespace Pulsestorm\\Travis;\n\nclass Foo\n{\n}\n"  > app/code/Pulsestorm/Travis/Test.php

-# pestle.phar magento2:generate:crud-model Pulsestorm_Travis Thing
+## test generate:crud-model, rm InstallSchema backup file
+pestle.phar magento2:generate:crud-model Pulsestorm_Travis Thing


I'm looking at this line -- am I correct that this PR is only concerned with the formatting on the following files?

app/code/Pulsestorm/Travis/Model/Thing.php app/code/Pulsestorm/Travis/Model/ThingRepository.php app/code/Pulsestorm/Travis/Model/ResourceModel/Thing.php app/code/Pulsestorm/Travis/Model/ResourceModel/Thing/Collection.php

9 files are generated, 8 if you remove the .bak file

app/code/Pulsestorm/Travis/Model/Thing.php
app/code/Pulsestorm/Travis/Model/ThingRepository.php
app/code/Pulsestorm/Travis/Model/ResourceModel/Thing.php
app/code/Pulsestorm/Travis/Model/ResourceModel/Thing/Collection.php

app/code/Pulsestorm/Travis/Api/ThingRepositoryInterface.php
app/code/Pulsestorm/Travis/Api/Data/ThingInterface.php
app/code/Pulsestorm/Travis/Setup/InstallSchema.php
app/code/Pulsestorm/Travis/Setup/InstallData.php

They all pass phpcs checks now.

In InstallData.php I disabled phpcs for the ->install() method (empty function)
I tried disabling just the specific rule ( // phpcs:disable Magento2.CodeAnalysis.EmptyBlock.DetectedFUNCTION ) it works locally but fails in Travis build.

astorm · 2019-08-14T15:28:21Z

travis/for-lint.sh

-# pestle.phar magento2:generate:crud-model Pulsestorm_Travis Thing
+## test generate:crud-model, rm InstallSchema backup file
+pestle.phar magento2:generate:crud-model Pulsestorm_Travis Thing
+rm app/code/Pulsestorm/Travis/Setup/InstallSchema.php.*.bak.php


+1 for getting rid of the backup files -- what do you think about making this a bit more generic. Something like ...

find app/code/Pulsestorm/Travis -type f -name '*.bak*' -exec rm '{}' \;

That'll get rid of any/all .bak files now and in the future.

sallaberry · 2019-08-16T13:29:35Z

Sorry for the delay, I made the requested changes.

astorm · 2019-08-17T18:26:55Z

Great work @sallaberry! And re:

Sorry for the delay, I made the requested changes.

You're offering your free time to improve this software project -- so you get to decide how long you take :)

Merging the changes now. I've added this soon-to-be-closed-PR to the "current undated sprint" milestone, and it will go out in the next release of pestle -- which should be soonish (probably when I finish up the remaining documentation items for pestle/internals over on https://pestle.readthedocs.io)

Also, semi related, this work led me to open a new issue: #491 -- I think I'd like to get those backup files out of the module folder itself and put somewhere else.

Michel Sallaberry added 10 commits August 8, 2019 18:50

Always an empty line after namespace, add phpdoc for class and interf…

01f8713

…aces, fix empty lines missing/unnecessary

templateConstruct _construct methods phpdoc + missing space after com…

add995c

…ma in _init argument list

templateConstruct _construct convert argument to ::class notation

8e67904

template Install / Upgrade function, install methods phpDocs

f3c8cfd

implements template : php-cs line exceed 120 characters : pretty mult…

596248f

…iline implements

php cs remove trailing comma in inline array + fix regex interface te…

6d524f9

…mplate + line > 120 characters install / upgrade methods + phpdocs for repository interface methods

php cs multiline init and whitespace fix

ba407ae

repository template phpdoc for all methods and sort use alphabetically

18e11cc

edit testCreateClassTemplate method to have phpdoc now

f3ef41b

test remove trailing comma inline array declaration

92f0e61

astorm mentioned this pull request Aug 9, 2019

Run PHPCS Tests #479

Merged

sallaberry and others added 2 commits August 10, 2019 23:59

Merge branch 'master' into astorm#476-Crud_Improvements

2a6c573

phpcs test crud-model

aff1be2

sallaberry and others added 8 commits August 11, 2019 05:39

Merge branch 'master' into astorm#476-Crud_Improvements

563fc1d

travis generate code AFTER pestle build

08c976b

wrong folder

d3413fd

wrong folder again..

870b91f

last 2 phpcs warnings

ab156cc

output console when backing up an already existing file

23ec105

check

04f88a9

I can't even type right

8a13037

Michel Sallaberry added 4 commits August 11, 2019 21:53

Show sniff codes in all reports

3c83441

phpcs stuff

5519cee

phpcs try again targeting specific rule

06463b4

back to working state, just disable phpcs completely for that section…

9b110ec

… of file

astorm added the ready for review label Aug 13, 2019

astorm requested changes Aug 14, 2019

View reviewed changes

sallaberry and others added 2 commits August 16, 2019 15:19

Merge branch 'master' into astorm#476-Crud_Improvements

517ee60

Remove all .bak files

386ffe0

astorm added this to the Current Undated Sprint milestone Aug 17, 2019

Merge branch 'master' into astorm#476-Crud_Improvements

2d851fb

astorm merged commit 5622bcf into astorm:master Aug 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#476 crud improvements #477

#476 crud improvements #477

sallaberry commented Aug 9, 2019

astorm commented Aug 9, 2019

sallaberry commented Aug 9, 2019

astorm commented Aug 10, 2019 •

edited

Loading

sallaberry commented Aug 10, 2019

sallaberry commented Aug 10, 2019 •

edited

Loading

astorm commented Aug 11, 2019

astorm commented Aug 11, 2019

sallaberry commented Aug 11, 2019

astorm commented Aug 11, 2019

astorm commented Aug 13, 2019

sallaberry commented Aug 13, 2019

astorm left a comment

astorm Aug 14, 2019

sallaberry Aug 16, 2019

astorm Aug 14, 2019

sallaberry commented Aug 16, 2019

astorm commented Aug 17, 2019

#476 crud improvements #477

#476 crud improvements #477

Conversation

sallaberry commented Aug 9, 2019

astorm commented Aug 9, 2019

sallaberry commented Aug 9, 2019

astorm commented Aug 10, 2019 • edited Loading

sallaberry commented Aug 10, 2019

sallaberry commented Aug 10, 2019 • edited Loading

astorm commented Aug 11, 2019

astorm commented Aug 11, 2019

sallaberry commented Aug 11, 2019

astorm commented Aug 11, 2019

astorm commented Aug 13, 2019

sallaberry commented Aug 13, 2019

astorm left a comment

Choose a reason for hiding this comment

astorm Aug 14, 2019

Choose a reason for hiding this comment

sallaberry Aug 16, 2019

Choose a reason for hiding this comment

astorm Aug 14, 2019

Choose a reason for hiding this comment

sallaberry commented Aug 16, 2019

astorm commented Aug 17, 2019

astorm commented Aug 10, 2019 •

edited

Loading

sallaberry commented Aug 10, 2019 •

edited

Loading