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

Fix: EventFake does not match Event:listen #54907

Conversation

pandiselvamm
Copy link
Contributor

@pandiselvamm pandiselvamm commented Mar 5, 2025

This PR fix this issue #54878

@pandiselvamm pandiselvamm marked this pull request as draft March 5, 2025 13:09
Copy link

github-actions bot commented Mar 5, 2025

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@taylorotwell
Copy link
Member

We never document this syntax.

@Sophist-UK
Copy link

Sophist-UK commented Mar 5, 2025

@taylorotwell

If you read #54878, you will see that @pandiselvamm has plagiarised my code and submitted a PR in his name without providing sufficient description and justification and without even bothering to credit my efforts.

Nevertheless...

  1. The existing code is clearly trying to match different syntaxes which effectively are the same event listener relationships in order to achieve what this PR is attempting to make work comprehensively, but the existing code is buggy. If you read EventFake does not match Event:listen documentation - both need fixing #54878, I provide a comprehensive set of tests that exhaustively demonstrate all the times this normalisation doesn't work.

  2. What syntax was "never documented"? The class@method syntax? If you read the code of this PR you will see that this syntax is already in use in e.g. the existing EventFaker test cases.

  3. Even if the syntax was never documented, it works in other places in the code, so for consistency it should also work here.

  4. Even if you don't want to fix the undocumented syntax here, this PR also fixes OTHER issues with the documented syntax.

  5. The code that exists here is actually smaller and more compact than the existing code, and therefore it doesn't contribute to code growth and has LESS code to maintain.

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.

3 participants