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

Don't show source location for logs that don't have that information #1027

Merged
merged 2 commits into from Jul 2, 2015

Conversation

Projects
None yet
5 participants
@andrewthad
Copy link
Contributor

andrewthad commented Jun 30, 2015

#1026

Don't merge this yet. I haven't tested it. Two questions:

  • There's a line in a haddock comment I added where I try to escape some special characters. Did I do this right?
  • Is there any reason we can't use <> instead of mappend? I'd much rather switch to that.
@gregwebs

This comment has been minimized.

Copy link
Member

gregwebs commented Jun 30, 2015

  • I am horrible at haddocks :)
  • <> was not introduced until GHC 7.4. Not sure if we know whether < 7.4 is broken yet.

Also, I would use a where binding instead of a let for sourceSuffix. That way it is clear it is only dependent on the function inputs.

@creichert

This comment has been minimized.

Copy link
Member

creichert commented Jun 30, 2015

  • You can cabal haddock (or stack haddock?) locally and open that specific file if you want to see it. It looks good to me, though.
  • It would be nice if the CI coverage could definitively tell us whether something like <> will cause breakage on supported GHC versions. I can't remember what the conclusion was the last time this was discussed.
@andrewthad

This comment has been minimized.

Copy link
Contributor Author

andrewthad commented Jun 30, 2015

Looks like stack haddock builds docs for all the dependencies too. Super cool. It looks like it ended up formatted correctly.

I'll leave mappend, just in case some poor soul is building yesod on GHC 7.2.

@gregwebs I like your suggestion about using a where binding. I've changed it to use that instead.

@andrewthad

This comment has been minimized.

Copy link
Contributor Author

andrewthad commented Jun 30, 2015

So, I'm trying to test this, and it builds correctly with stack, but I want to make sure that it has the expected runtime behavior. I created a minimal yesod+sqlite single-file app named example.hs, but I can't figure out how to run it:

# Neither of these work
stack ghc example.hs
stack exec runhaskell example.hs

Everything I try gives:

example.hs:11:8:
    Could not find module ‘Yesod’
    Use -v to see a list of the files searched for.

Does anyone have any suggestions for using a library you build with stack?

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jun 30, 2015

You probably are using a different snapshot. Try running stack build yesod first and see if that changes the result.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented Jun 30, 2015

And FYI, there's also stack runghc.

@andrewthad

This comment has been minimized.

Copy link
Contributor Author

andrewthad commented Jul 1, 2015

Thanks Michael. I had forgotten that yesod-core doesn't export Yesod (the module), so I basically needed to build more.

Something weird is happening. The changes that I've introduced strip off the piece at the end for anything that uses logDebugN, etc. However, this does not fix the SQL logs. I put some additional debugging logic around it and discovered that yesod's messageLoggerSource isn't even called for SQL logs. So, does anyone know where they are getting formatted or why they are immune from the normal logging process?

@andrewthad

This comment has been minimized.

Copy link
Contributor Author

andrewthad commented Jul 1, 2015

Wait a moment. I think I know why.

@andrewthad

This comment has been minimized.

Copy link
Contributor Author

andrewthad commented Jul 1, 2015

Nevermind, everything is good, and this should be ready to be merged in. I was building one of the example apps from the yesod book. It uses runStderrLoggingT for the SQL logs, which cuts yesod out of the equation and uses monad-logger's defaultLogStr instead, which produces very similar (maybe even the same) default logs as yesod does. Maybe that one should be changed too, but I'll leave that for another day.

@andrewthad

This comment has been minimized.

Copy link
Contributor Author

andrewthad commented Jul 2, 2015

This is ready to be merged in. It has the expected behavior.

@geraldus

This comment has been minimized.

Copy link
Contributor

geraldus commented Jul 2, 2015

Cool stuff! These <unknown> things are very confusing to me.

snoyberg added a commit that referenced this pull request Jul 2, 2015

Merge pull request #1027 from andrewthad/master
Don't show source location for logs that don't have that information

@snoyberg snoyberg merged commit 56ad4b1 into yesodweb:master Jul 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.