Refactor Format class #3563
Refactor Format class #3563
Conversation
|
Good job, my PR just missed tests, I asked for help with it but you know. .. |
There was a problem hiding this comment.
Nice work! You should definitely learn how to resolve merge conflicts, a worthwhile skill. Read on using --theirs and --ours to keep whole files, then it's mostly just going through files manually and resolving the ======= lines. @samsonasik made a nice blog entry on it recently.
Couple notes on this. I'd like to get @lonnieezell 's opinion on the namespace formatting so we can commit to those globally.
| @@ -1,54 +1,59 @@ | |||
| <?php namespace Config; | |||
There was a problem hiding this comment.
We're currently all over the place on these. Let's see if @lonnieezell wants to make a call on the styling of namespaces before we change them.
There was a problem hiding this comment.
I agree with this. Currently it's a mix, and we need a definitive style guide for this one.
There was a problem hiding this comment.
My personal preference has always been on line 1, but it seems my IDE and lots of other programmers prefer it with a blank space after the opening php tag. I'm fine with either of those options. Not of fan of having the CI docblock above it, though. I think namespace should be the first item in the file.
So do either of you have preferences on first line vs third line?
There was a problem hiding this comment.
I've been doing it on first line but I think I picked that up from you ;) I don't use an IDE so I'm not sure how much work it is to fight against one. Are there any standards? Does our CS have options that might indicate a "default"?
There was a problem hiding this comment.
I am following PSR-12.
When the opening <?php tag is on the first line of the file, it MUST be on its own line with no other statements unless it is a file containing markup outside of PHP opening and closing tags.
There was a problem hiding this comment.
I'm good with following the PSR on this. I had forgotten 12 got released, actually, so thanks for the reminder.
There was a problem hiding this comment.
Oh that's great! Yes we should definitely move towards that then. The consensus is third line for namespace then?
There was a problem hiding this comment.
Yes. Third line for namespace if no file-level doc block. Namespace after doc block if present. This is following PSR12.
I'll take note on this on my upcoming PR on php-cs-fixer fixes.
There was a problem hiding this comment.
Bummer - namespace after file docblock is in PSR. Oh well. Can't have it all :)
|
@paulbalandan is this ready? |
I dunno. Is there more to add here? It's ready regardless. Sorry, I'm weak at colloquial English so I don't quite get what Lonnie last meant. 😂 |
|
I think it is good! That conversation resolved. "Bummer" = that is unfortunate "Can't have it all" = you cannot get everything you want |
|
Thanks to @paulbalandan and @mostafakhudair for this one! |
Description
Closes #3149 .
I tried to pull that PR and continuing from that but when I rebased to latest develop I got merge conflicts. I am not yet adept in solving merge conflicts, even IDE-aided, so I opted for the long road. This PR also adds tests for the new class addition and new inserted function to
Services.Checklist: