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

display trace/warning messages #132

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Conversation

daniel-sampliner
Copy link
Contributor

Fixes issue #128.

@daniel-sampliner
Copy link
Contributor Author

I'm a definitely a complete novice to Haskell, so I won't deny this PR carries the stench of a copy-and-paste hack job. No offense taken if this is rejected in favor of a cleaner solution.

@daniel-sampliner daniel-sampliner marked this pull request as ready for review April 2, 2024 14:20
@maralorn
Copy link
Owner

maralorn commented Apr 2, 2024

Don’t worry. Thank you for looking into this. And I am glad that you learned a bit of Haskell along the way. :D

I will look into it as soon as I got the time.

@maralorn
Copy link
Owner

maralorn commented Apr 2, 2024

The Haskell code looks quite fine to me. Only question is what is operationally the "correct" solution. Does it make sense to special case messages prepended with "trace"? Are there other similar messages which we should also display?

@daniel-sampliner
Copy link
Contributor Author

Does it make sense to special case messages prepended with "trace"? Are there other similar messages which we should also display?

Yeah I was concerned about the same. I was hoping that these kind of messages would have a unique level in the json logs (or any other concrete signature), but unfortunately that doesn't appear to be the case.

That being said, is it possible to print messages to the console outside of builtins.trace (or throwing an error, but I think we can safely say errors are already covered by nom)? There isn't anything like printf, so I imagine any kind of abstractions in nixos modules and the various lib.traceX are all eventually going to call builtins.trace?

@maralorn
Copy link
Owner

maralorn commented Apr 2, 2024

Thank you. This is an improvement as is.

@maralorn maralorn merged commit 5cc29ee into maralorn:main Apr 2, 2024
@viperML
Copy link

viperML commented Apr 4, 2024

Could we get a new release tag that includes this PR? Or maybe something else blocking on master?

@maralorn
Copy link
Owner

maralorn commented Apr 4, 2024

There is a test failing on the main branch and I haven’t had the patience to look into it.
Also I want to fine tune and improve the trace display a bit before I release it.
So this might take a bit, sorry.

@viperML
Copy link

viperML commented Apr 4, 2024

No problem, just wanted to know about that 🙂

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

4 participants