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

Extend logs for warnings #982

Merged
merged 3 commits into from
Jan 9, 2023
Merged

Extend logs for warnings #982

merged 3 commits into from
Jan 9, 2023

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Jan 8, 2023

  • split logs into a separate module
  • make some logs warnings and fatal (to be continued...)
  • prepare failure logging infrastructure for TUI

This should be merged before #954 so it is not squashed.

@xsebek xsebek requested review from kostmo and byorgey January 8, 2023 17:37
@@ -109,8 +109,8 @@ handleEvent = \case
AppEvent (UpstreamVersion ev) -> do
let logReleaseEvent l e = runtimeState . eventLog %= logEvent l ("Release", -7) (T.pack $ show e)
case ev of
Left e@(FailedReleaseQuery _e) -> logReleaseEvent ErrorTrace e
Left e -> logReleaseEvent Said e
Copy link
Member Author

Choose a reason for hiding this comment

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

@kostmo @byorgey FYI: this used to create nice yellow warning logs by abusing the fact that messages said by the robot with ID -7 happen to be in yellow. 😅

Copy link
Member

@byorgey byorgey 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!

src/Swarm/Game/Log.hs Outdated Show resolved Hide resolved
-- ^ The ID of the robot that generated the entry.
, _leLocation :: Location
-- ^ Location of the robot at log entry creation.
, _leText :: Text
Copy link
Member

Choose a reason for hiding this comment

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

At #959 (comment) we discussed maybe storing something more structured here than just a Text, which I think is worth exploring. But we should probably leave that to a different PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to split a separate type for system logs which should also use structured errors.

I think that will make the design clear, now it's all tangled and adding anything to it would be a mistake IMO.

src/Swarm/TUI/Model/Failure.hs Outdated Show resolved Hide resolved
src/Swarm/TUI/Model/Failure.hs Outdated Show resolved Hide resolved
xsebek and others added 3 commits January 9, 2023 00:59
* split logs into separate module
* make some logs warnings and fatal (to be continued...)
* prepare failure logging infrastructure for TUI
Co-authored-by: Brent Yorgey <byorgey@gmail.com>
@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Jan 9, 2023
@mergify mergify bot merged commit d5a86c0 into main Jan 9, 2023
@mergify mergify bot deleted the failure branch January 9, 2023 00:10

-- | An entry in a robot's log.
data LogEntry = LogEntry
{ _leTime :: Integer
Copy link
Member

Choose a reason for hiding this comment

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

These fields sound French 😜

@kostmo kostmo mentioned this pull request Feb 18, 2023
@xsebek xsebek mentioned this pull request Feb 20, 2023
mergify bot pushed a commit that referenced this pull request Feb 22, 2023
closes #1115

This PR makes use of the rich types defined in #982 and converts many functions of `Has (Throw Text)` type to `ExceptT`.

It also logs a warning instead of crashing with terminal failure if "save" info cannot be loaded, which will be important for #974 where the save file schema changes.

![Screenshot from 2023-02-18 13-22-16](https://user-images.githubusercontent.com/261693/219901189-c9d97fca-698a-4080-84fc-0b4110d3a601.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants