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

One file per achievement record #954

Merged
merged 3 commits into from
Jan 9, 2023
Merged

One file per achievement record #954

merged 3 commits into from
Jan 9, 2023

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Jan 4, 2023

Closes #953

Testing

First, earn a couple of achievements by playing the game. Then, stuff some garbage into the achievement directory and rerun the game:

kostmo@dell-ubuntu:~/github/swarm$ touch ~/.local/share/swarm/achievement/foo
kostmo@dell-ubuntu:~/github/swarm$ mkdir ~/.local/share/swarm/achievement/bar
kostmo@dell-ubuntu:~/github/swarm$ stack run
Aeson exception:
Error in $: parsing Swarm.TUI.Model.Achievement.Attainment.Attainment(Attainment) failed, expected Object, but encountered Null
Entry "/home/kostmo/.local/share/swarm/achievement/bar" is not a file!

Observe that the game starts fine and is able to display our original 2 achievements.

kostmo
kostmo commented Jan 4, 2023
View reviewed changes
src/Swarm/TUI/Model/Achievement/Persistence.hs Outdated
(</> "achievements") <$> getSwarmDataPath createDirs
getSwarmAchievementsPath createDirs = do
dp <- getSwarmDataPath False
let achievementsDir = dp </> "achievement"
Copy link
Member Author

@kostmo kostmo Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I changed the name from achievements to achievement so that any recent players of the game don't get an error due to that path not being a directory.

@kostmo kostmo requested review from byorgey and xsebek January 4, 2023 07:33
@kostmo kostmo marked this pull request as ready for review January 4, 2023 07:33
byorgey
byorgey approved these changes Jan 4, 2023
View reviewed changes
src/Swarm/TUI/Model/Achievement/Persistence.hs Outdated Show resolved Hide resolved
xsebek
xsebek reviewed Jan 4, 2023
View reviewed changes
src/Swarm/TUI/Model/Achievement/Persistence.hs Outdated
]
let (bads, goods) = partitionEithers eithersList
forM_ bads $ \b ->
hPutStrLn stderr $ T.unpack b
Copy link
Member

Choose a reason for hiding this comment

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

Please don’t put stuff to stderr.

We have a RuntimeState with logs for this purpose.

Let me describe why:

  1. I run the game
  2. an error is printed shortly before Brick opens
  3. I play Swarm puzzled why some things are wrong
  4. I close the game and see the log
  5. I wonder why I am only seeing this now and not in game notifications

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, let's log using the existing mechanism. Could you please illustrate how? I've pushed a commit atop this branch with an incomplete attempt.

@restyled-io restyled-io bot mentioned this pull request Jan 4, 2023
Closed
@xsebek
Copy link
Member

xsebek commented Jan 8, 2023 •
edited
Loading

@kostmo I will fix #941 and then get to this, there will be a small conflict I assume. 🤷‍♂️

For the record I thought achievements was a directory given the plural and no file extension, so my fix was wrong.

@xsebek
Copy link
Member

xsebek commented Jan 8, 2023

@kostmo I hope you won't mind if I rebase this and force-push with the fix of the WIP commit. 🙂

You got it 99% right, it's just that in initAppState the monad does not give us state updates on RuntimeState, so we need to use "classic" lenses and folds. 🧑‍🏭

@xsebek
Copy link
Member

xsebek commented Jan 8, 2023 •
edited
Loading

@kostmo I got carried away and refactored the logging a little.

I will move d8def97 to a separate PR so it does not get squashed here with the other changes.

EDIT: @kostmo please merge #982 before this one. 🙂

@xsebek
Copy link
Member

xsebek commented Jan 8, 2023

With file achievement:
image
With bogus foo and bar:
Screenshot from 2023-01-08 18-34-45

@xsebek xsebek mentioned this pull request Jan 8, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jan 9, 2023
@xsebek
Extend logs for warnings (#982)
d5a86c0 
* 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 added merge me Trigger the merge process of the Pull request. and removed merge me Trigger the merge process of the Pull request. labels Jan 9, 2023
@xsebek
Copy link
Member

xsebek commented Jan 9, 2023

@kostmo if you are satisfied with this, feel free to merge. 🙂

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Jan 9, 2023
@mergify mergify bot merged commit 5e774fa into main Jan 9, 2023
@mergify mergify bot deleted the file-per-achievement branch January 9, 2023 00:35
@xsebek
Copy link
Member

xsebek commented Jan 9, 2023

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xsebek xsebek xsebek left review comments

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Graceful handling of unrecognized achievements
3 participants
@kostmo @xsebek @byorgey