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

PSR Logger plugin messages accuracy improvements #75

Closed
wants to merge 3 commits into from

Conversation

gquemener
Copy link
Contributor

Several messages were in the present tense whereas they should be in the past tense.

The logged message was inaccurate in case of an event dispatching:

before
> Dispatching event "App\Acme\Domain\MyEvent" to handler

after
> Dispatching event "App\Acme\Domain\MyEvent" to listeners "foo, bar, baz"

I've also wrapped all message names with double quotes.

@coveralls
Copy link

coveralls commented Jun 30, 2018

Pull Request Test Coverage Report for Build 245

  • 5 of 16 (31.25%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 84.407%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/MessageContext/ContextFactory.php 5 6 83.33%
src/Plugin/PsrLoggerPlugin.php 0 10 0.0%
Totals Coverage Status
Change from base Build 244: -0.2%
Covered Lines: 406
Relevant Lines: 481

💛 - Coveralls

@codeliner codeliner requested a review from UFOMelkor July 15, 2018 18:17
@gquemener gquemener force-pushed the feature/improve-psr-logger branch from b3156b1 to 1341e59 Compare October 4, 2019 07:04
@gquemener
Copy link
Contributor Author

Hello,

I've just rebased this branch to include earliest master commits.

Have you had the chance to give some thoughts about it @UFOMelkor ?
Please let me know if anything disturbs you, so that I can work on it.

Thanks!

@codeliner
Copy link
Member

codeliner commented Oct 5, 2019

@gquemener Do you know how to fix the failing tests? Can be done in a separate PR, if you think that's better

Edit: Ok, did not see your issue: #79
I'm also out of ideas how to solve that :(

@gquemener
Copy link
Contributor Author

@codeliner I think it's worth it to deal with the issue #79 in another PR. My guess is that it's a mix of Symfony / Prooph bundle bugs.

Furthermore, the testsuite is already broken on master, so merging this PR will not hurt.

I keep investigating.

@gquemener gquemener force-pushed the feature/improve-psr-logger branch from 1341e59 to 5559b0b Compare October 11, 2019 17:29
@codeliner
Copy link
Member

🤔 @gquemener I've merged the open PRs and fixed a conflict in this PR. My hope was that pipeline is green now but master is broken as well as your PR. Any ideas?

Would like to do one last release for this package, before we stop supporting it.

@gquemener
Copy link
Contributor Author

Hello @codeliner,

The whole testsuite is broken due to some changes within Symfony DI. It looks like these changes were not BC, and I am not sure if this is a problem in the version constraints that are set in the composer.json or problems within this bundle.

However, I've started to fill holes on a branch. I guess I need a few weeks to fix all this mess on my spare time. How long d'you give me? ^^

@gquemener
Copy link
Contributor Author

#83

@gquemener
Copy link
Contributor Author

Closing due to inactivity

@gquemener gquemener closed this Feb 23, 2021
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