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.

Sign up for GitHub

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

Jump to bottom

First attempt at logging to files instead of std out #135

Merged
merged 7 commits into from
Sep 26, 2024
Merged

First attempt at logging to files instead of std out #135

merged 7 commits into from
Sep 26, 2024

Conversation

The1Penguin
Copy link
Contributor

No description provided.

SwedishSubmarine
SwedishSubmarine approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@SwedishSubmarine SwedishSubmarine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Not sure how to unit test this but shouldn't break anything either way so worth trying.

Rembane
Rembane requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@Rembane Rembane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn good initiative if I may say so myself!

See the comments for confusion, hearts and questions!

app/Main.hs
@@ -37,6 +37,7 @@ import System.Console.GetOpt ( ArgDescr(..)
, getOpt
, usageInfo
)
import System.Directory ( createDirectoryIfMissing )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 IT EXISTS FOR REAL! 🎉

app/Main.hs Outdated
@@ -54,6 +55,7 @@ opts :: [OptDescr (Config -> Config)]
opts =
[ Option [] ["help"] (NoArg (set cHelp True)) "Show usage info"
, Option [] ["port"] (ReqArg (set cPort . read) "PORT") "Port to run on"
, Option [] ["path"] (ReqArg (set cLogPath) "PATH") "Path to save log files to, default is 'logs'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, Option [] ["path"] (ReqArg (set cLogPath) "PATH") "Path to save log files to, default is 'logs'"
, Option [] ["path"] (ReqArg (set cLogPath) "PATH") "Path to save log files to, default is 'logs'"

Columns <3

app/Main.hs
@@ -71,6 +73,7 @@ main = (recreateConfig . getOpt Permute opts <$> getArgs) >>= \case
(config , _ , _ ) -> do
upd <- newEmptyMVar -- putMVar when to update
viewRef <- createViewReference
createDirectoryIfMissing True (_cLogPath config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this path relative?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both absolute and relative that is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It handles both :D 🗡️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOME! I am now a much calmer human.

src/Model.hs Outdated
import Data.AffineSpace ( (.+^) )
import Data.Thyme ( _localDay
, _localTimeOfDay
, _todHour
, _zonedTimeToLocalTime
, getZonedTime
, getZonedTime, getCurrentTime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, getZonedTime, getCurrentTime
, getCurrentTime
, getZonedTime

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newlines and lexicographic ordering <3

Multipacker
Multipacker approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@Multipacker Multipacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a minor indentation fix

src/Model.hs Outdated
Comment on lines 54 to 55
import Model.Linsen
import Text.Printf (printf)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor indentation fix

Suggested change
import Model.Linsen
import Text.Printf (printf)
import Model.Linsen
import Text.Printf (printf)

@Rembane
Copy link
Contributor

Rembane commented Sep 25, 2024

Something just struck me, this is me thinking aloud, I think it's faster and uses less resources to write to file as a ByteString or Text, OTOH I think we'll have to go through String anyway, so maybe it doesn't matter?

@The1Penguin
Copy link
Contributor Author

Something just struck me, this is me thinking aloud, I think it's faster and uses less resources to write to file as a ByteString or Text, OTOH I think we'll have to go through String anyway, so maybe it doesn't matter?

Ignored 🗡️

@Rembane Rembane self-requested a review September 26, 2024 09:36
Rembane
Rembane approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@Rembane Rembane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done art! You can use it if you want to. 😸

@The1Penguin The1Penguin merged commit 2a74f3d into main Sep 26, 2024
1 check passed
@The1Penguin The1Penguin deleted the better-error-messages branch September 26, 2024 09:42
@The1Penguin The1Penguin mentioned this pull request Sep 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rembane Rembane Rembane approved these changes

@Multipacker Multipacker Multipacker approved these changes

@SwedishSubmarine SwedishSubmarine SwedishSubmarine approved these changes

@rachelambda rachelambda Awaiting requested review from rachelambda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@The1Penguin @Rembane @Multipacker @SwedishSubmarine