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

Addressing #639: print stacktrace in logs #658

Closed
wants to merge 53 commits into from
Closed

Addressing #639: print stacktrace in logs #658

wants to merge 53 commits into from

Conversation

henryxparker
Copy link

@henryxparker henryxparker commented May 18, 2023

This is a quick and dirty fix for the weaver.Log not printing stack traces.
I didn't see any tests for the core module so I didn't write any.
Any feedback appreciated

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 7 committers have signed the CLA.

✅ Baccata
✅ henryxparker
❌ zetashift
❌ valencik
❌ armanbilge
❌ CJSmith-0141
❌ zainab-ali


zainab-ali seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Henry Parker seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@henryxparker henryxparker changed the title Addressing [issue 639](https://github.com/disneystreaming/weaver-test/issues/639): print stacktrace in logs Addressing #639: print stacktrace in logs May 18, 2023
@henryxparker
Copy link
Author

henryxparker commented May 19, 2023

I don't know why the cla assistant has two comments. Let me know if that is a blocker and what to do about it

Edit: committer and author were different in git, but they're both me, so I changed the commit to reflect that and the second comment can be ignored. I can make a new PR if necessary

@Baccata
Copy link
Contributor

Baccata commented Sep 27, 2023

Hey @henryxparker, sorry for putting some eyes on this only now, things were crazy at work when you raised the PR and I totally forgot to go back to it, apologies.

Anyway, the best location to add tests for this feature would probably be here. Chances are you should probably handcraft a stacktrace to make it easy to compare results and expectations

zainab-ali and others added 23 commits March 8, 2024 17:26
Migrate build to sbt-typelevel
Understandably the IntelliJ section was removed. This portion of the
documentation is still very useful, especially for people who are used
to being able to hit the play button on individual tests.
The old intellij sections has been moved to "Frequently Asked Questions".
Fix organization and artifact names
Adds `.only` and `.ignore` documentation.
@henryxparker
Copy link
Author

Sorry for the long wait. I was laid off for a while and totally forgot that I had made this PR.
Now that everything is settled I'll be working on it again

@Baccata
Copy link
Contributor

Baccata commented Mar 20, 2024

@henryxparker thanks for taking the time to come back to this PR !

However, weaver is in the process of being moved to the typelevel organisation, and though I cannot attest to when exactly the move will conclude, it's quite unlikely that I'll issue another release of weaver under the disneystreaming organisation. If it's not too much to ask, I would suggest moving your PR over there.

https://github.com/typelevel/weaver-test

@henryxparker
Copy link
Author

Oh sweet, I'll add it there. Could you add something to the README to indicate this move?

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.

8 participants