Skip to content
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

#242 #260

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

#242 #260

wants to merge 4 commits into from

Conversation

WraithKenny
Copy link

Doing external file gets for a version number that hasn't changed in 4 years is more than a waste, it creates crashes, when all we need to do is update the static if it ever changes.

Doing external file gets for a version number that hasn't changed in 4 years is more than a waste, it creates crashes, when all we need to do is update the static if it ever changes.
@WraithKenny
Copy link
Author

Fixes #242

@dingo-d
Copy link
Member

dingo-d commented Oct 15, 2020

I personally don't have anything against it. @jrfnl what was the idea behind this sniff?

@dingo-d
Copy link
Member

dingo-d commented Feb 20, 2021

@WraithKenny can you just rebase this PR so that we see if the checks are passing on GH Actions? Thanks

@WraithKenny
Copy link
Author

@dingo-d I merged lastest into patch-1

@dingo-d dingo-d added this to the 0.2.x/0.3.0 Next milestone Jun 16, 2021
@jrfnl
Copy link

jrfnl commented Jun 22, 2021

I personally don't have anything against it. @jrfnl what was the idea behind this sniff?

@dingo-d By the looks of it, the sniff itself hasn't changed, it's just the external call to the GH API to get the version number of the latest release of TGMPA which is being removed.

I'm fine with that (for now). At the time I added it, it was expected that there would be more regular TGMPA releases. The problem this code solved was that the sniff would have to be updated after every TGMPA release to make sure it would check for the latest release and then, of course, users would need to update their install of TRTCS regularly as well, as otherwise the sniff would still check against outdated version numbers.
Removing this code, reintroduces that problem, but as stated above, there hasn't been a release of TGMPA for a number of years, so in practice, it's not a big thing at this moment.

If I ever find the time to do more regular maintenance of TGMPA again and we'd be releasing regularly again, we can always revisit and re-introduce this code.

@dingo-d
Copy link
Member

dingo-d commented Jun 22, 2021

@WraithKenny there is just one small error I'm seeing in the actions run

FILE: WPThemeReview/Sniffs/Plugins/CorrectTGMPAVersionSniff.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 203 | ERROR | Empty line not required before block comment
     |       | (Squiz.Commenting.BlockComment.HasEmptyLineBefore)
----------------------------------------------------------------------

Can you fix this and then I can merge this PR 👍🏼 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants