Preferred method for converting framework to PSR-2

89 views
Skip to first unread message

Christopher Pitt

unread,
Mar 16, 2016, 5:41:47 PM3/16/16
to SilverStripe Core Development
Hi folks,

This is mostly a question for the core committers: do you think it would be better to convert the whole codebase at once, or take a more iterative approach? This won't mitigate the blame problem, but it will make things easier to review IMHO. If this is the preferred way to go, I would still suggest a final "convert the remaining things to PSR-2 pull request", but that will be significantly smaller and easier to review.

Kind regards
Chris

Loz Calver

unread,
Mar 17, 2016, 5:47:28 AM3/17/16
to SilverStripe Core Development
I’d personally prefer to see changes grouped together rather than one commit per file, else we’ll have hundreds of commits in our history that are mostly whitespace changes. Exactly how they’d be grouped I’m not sure - it could be by directory with exceptions for complex files like DataObject.php, or perhaps just a few files at a time - but I don’t think that matters too much as long as it’s not too impossible to review.

Regarding blame view, let’s just cross our fingers and hope that Github eventually add ?w=0 support! If we find ourselves needing the blame view for anything, we should still be able to do it on command line with the -w option right? We’ll probably wipe this out if we change our directory structure when implementing namespaces anyway...

Loz

Hamish Friedlander

unread,
Mar 17, 2016, 4:33:22 PM3/17/16
to silverst...@googlegroups.com
I prefer whole-codebase-at-once. Automated conversion of whitespace (and other bits that can be automatically converted) followed by seperate iterative cleanup of stuff that can't be. Otherwise .editorconfig is going to be wrong for some files at least.

(And yes, lack of blame makes me sad, but doing file-at-a-time won't really fix that).

Hamish Friedlander

Sam Minnée

unread,
Mar 17, 2016, 5:04:05 PM3/17/16
to silverst...@googlegroups.com
If it's possible, my preference would probably 1 correction-mechanism at a time, across all files. Possibly as a series of commits in a single PR. The rationale is that it's easier to focus our review effort on the riskier steps. For example, whitespace changes are probably fine as long as the tests pass.

I'm not sure how granular the tooling is to support this, or how much extra effort it adds, though.

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.
--
Sam Minnée
CEO
SilverStripe Limited

Daniel Hensby

unread,
Mar 21, 2016, 1:01:49 PM3/21/16
to SilverStripe Core Development
+1 for all at once

Nicolaas Thiemen Francken - Sunny Side Up

unread,
May 2, 2016, 1:36:55 AM5/2/16
to silverstripe-dev
What is the best way to convert a module to psr-2?

I was thinking about using: http://cs.sensiolabs.org/ but there are probably a few things I do not want to change (e.g. MyModule::get()->first() should probably not be converted to self::get()->first() ) .


Ralph Slooten

unread,
May 2, 2016, 5:46:04 AM5/2/16
to silverstripe-dev
Have a look at https://github.com/FriendsOfPHP/PHP-CS-Fixer - very configurable.

On 2 May 2016 at 17:36, Nicolaas Thiemen Francken - Sunny Side Up <nfra...@gmail.com> wrote:
What is the best way to convert a module to psr-2?

I was thinking about using: http://cs.sensiolabs.org/ but there are probably a few things I do not want to change (e.g. MyModule::get()->first() should probably not be converted to self::get()->first() ) .


Martijn

unread,
May 4, 2016, 9:18:14 AM5/4/16
to SilverStripe Core Development

Sam Minnée

unread,
May 4, 2016, 5:39:02 PM5/4/16
to SilverStripe Core Development
Having thought about this more, I think that we should have:

 - 1 commit for all the whitespace changes (no trailing space, spaces to tabs)
 - 1 commit for all the automated style changes (a little bit of whitespace correction would be okay here, just not every-single-line)
 - Separate commits (or even numbers of them) for more involved changes (put all classes in namespaces, rename methods, etc)

This will make it easier to ignore the whitespace changes that will inevitably cause merge complexity from 3->4.

The other question is when? I think we should tie it to a specific release:

I think it comes down to the following choices:

 - Just before alpha1
 - Just after alpha1
 - Just before alpha2
 - Just after alpha2
 - Just before beta1

I'd want to get the views of the people who do most of the cross-release-branch merging (from memory I think Damian, Ingo, and Dan Hensby do a lot of this, sorry if I've neglected anyone else) as it will have a material impact on that work.

Thanks,
Sam

On Thu, 5 May 2016 at 01:18 Martijn <marv...@gmail.com> wrote:
or https://styleci.io/


--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

Ingo Schommer

unread,
May 4, 2016, 11:31:13 PM5/4/16
to SilverStripe Core Development
Whitespace and low impact syntax fixes should be a few days after alpha1, since we can't afford any unnecessary conflict resolutions before alpha1 to hit the release timing.

For semantic changes like intro of namespaces, I'd suggest the same timing (after alpha1). Basically depends on how stable any upgrade scripting is (Sam is working his magic there). That means module authors will need to adjust their code to work with namespaces in order to gain compact with alpha2 a few weeks later. Hence too high impact for an alpha1 change.

Sam Minnée

unread,
May 4, 2016, 11:39:04 PM5/4/16
to SilverStripe Core Development
Perhaps we should do an alpha2 release as soon as the namespacing and minor PSR2 clean-up is complete, and maybe perform any module splits at that time too?

Ingo Schommer

unread,
May 5, 2016, 4:17:59 AM5/5/16
to silverst...@googlegroups.com
Yeah that sounds good!
Reply all
Reply to author
Forward
0 new messages