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

@trigger_error deprecation sniff is too strict - allow the message to start with a variable #154

Merged
merged 2 commits into from Feb 21, 2022

Conversation

longwave
Copy link
Contributor

…trict - allow the message to start with a variable
@jonathan1055
Copy link
Contributor

I can see that you had to adjust the list of warnings and errors in FunctionTriggerErrorUnitTest.php which is a pain, as none of those lines are affected, it's just that the line numbers all shift when new 'ok' rows are added. When I wrote the sniff and the tests, I would not have done it that way if I knew now that it would mean altering the number. It would have been better if I'd had three separate test files, one for passes, one for errors and one for warning, and then new lines could always be added at the end.

I will see what @klausi says, but after this issus is done, I'd be happy to reorganise the test files. This wont be the last time this thing happens. I also have clashing changes on #153 so might wait for that one too.

@klausi
Copy link
Collaborator

klausi commented Feb 21, 2022

Thanks - merged #153 , can you resolve the conflicts here?

The number changes don't look so bad to me, I was able to understand them :)

@jonathan1055
Copy link
Contributor

I caused a conflict with #153 I will fix it

@jonathan1055
Copy link
Contributor

jonathan1055 commented Feb 21, 2022

I don't think I can push to longwave:relax-deprecation so I'm not sure of the best way to fix the conflict without creating my own branch of this work and a hence a new PR. There should be a way to do it directly, it is my lack of understanding of github

@klausi
Copy link
Collaborator

klausi commented Feb 21, 2022

Pull requests on Github are always owned by their creator, so you will have to make your own unfortunately.

@longwave
Copy link
Contributor Author

I merged in the changes from #153.

@jonathan1055
Copy link
Contributor

Thanks @longwave sorry I caused you that.

@klausi klausi merged commit 260ed68 into pfrenssen:8.3.x Feb 21, 2022
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