-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new upstream sniffs #240
base: develop
Are you sure you want to change the base?
Conversation
58a1a89
to
c445642
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of things:
- As this repo supposedly uses SemVer, adding new sniffs should not be done in a patch release. These should be milestoned for the next minor or this upcoming release should become
0.3.0
. EscapedNotTranslated
is an obvious one as it alerts people to a code error, so I don't think there can be any discussion over it, so 👍 from me.DisallowShortTernary
should probably be pulled separately and discussed in the Theme Review meeting.
There are good reasons to forbid it for Core, but I'm not sure this should be forbidden for themes and I can imagine this could yield a lot of discussion.
Possibly this should be downgraded to a warning for TRT with a custom message.- There is one more new sniff
WordPress.DateTime.CurrentTimeTimestamp
and a second group in theWordPress.DateTime.RestrictedFunctions
sniff.
Any particular reason why those haven't be pulled (either in this PR or in a separate one) ?
As themes will often display post/comment dates etc, I imagine them doing that with the correct timezone and such would be helpful.
I presume you didn't pull the PostTypeSlug one as registering post types is not allowed in themes ? Or am I remembering that wrong ?
Valid points! Will put this for next major version milestone 👍 Good point about shorthand ternary. Tbh I almost never used it (I use null coalesce most of the time), and from what I've seen by searching the theme directory (link), they are using it wrong - checking the existence of key in array with short ternary will either throw an error or
The first one I think I wasn't 100% sure if this is allowed in a theme (from your comment it looks like it has a merit of being in the theme).
So I didn't include it 😄
Correct 👍 Themes cannot register post types (this is covered by the |
It didn't belong in the other PR as it would change the functionality in TRTCS, while that PR was just about updating the WPCS dependency. For this PR, which is intended to change the TRTCS functionality, it would be a good fit. |
Basically, it's about decision points: By having the extra datetime check in that same PR, that PR would be asking for two decisions: "should we update the minimum WPCS version to 2.2.0 ?" AND "should this new check be added to TRTCS ?" As those are two different decisions which may have different answers, they shouldn't be in the same PR. To bring the point home: the first decision is something which could go into a patch version, the second decision means that PR would have to go into the next minor (as discussed above). Does that help ? |
Yes, it makes sense. I've moved the milestone so we can work on this after 0.2.1 is released 🙂 |
c445642
to
5bcd7bf
Compare
Detached the two new sniffs from #239 and add them here.
Short ternary addition seems like it promotes best practices, and escaped not translated sniff seems like a useful sniff for themes to have.