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

[General] Logging & launching improvements #1548

Merged
merged 11 commits into from
Jul 9, 2022
Merged

[General] Logging & launching improvements #1548

merged 11 commits into from
Jul 9, 2022

Conversation

CommandMC
Copy link
Collaborator

Main changes:

  • Logs are now live (meaning new output is written to the file immediately instead of waiting for game exit)
  • If an error happens, part of the log is still written (as much info as possible is provided, to help debug the issue)
  • Errors themselves are now logged with a stacktrace (not too helpful in production since function & file names are minified, but might help in development)
  • The "Error log"/"Game log" (stdout & stderr) split was removed, it's both in "Game log" again now
  • The log now contains a timestamp

Other changes:

  • getRunnerCallWithoutCredentials is exported again since it's needed for the new log behavior
  • Some code cleanup, didn't bother to open a separate PR for just that

Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

CommandMC added 5 commits July 4, 2022 23:33
@CommandMC
Misc: Several small optimizations
396674d
@CommandMC
Expose getRunnerCall again
9c261bb
@CommandMC
Trow more
9c46e99 
I think we should avoid this kind of "silent fail" syntax in the future,
it leads to not-that-clear error messages for the end user
@CommandMC
Remove old error message logic
eee9e63 
This is already done in the backend now
@CommandMC
Launching & logging improvements
6073fa3 
- Logs are now live (meaning new output is written to the file
  immediately instead of waiting for game exit)
- If an error happens, part of the log is still written
- Errors themselves are now logged with a stacktrace (not too helpful
  in production since function & file names are minified, but might
  help in development)
@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Jul 4, 2022
@CommandMC CommandMC self-assigned this Jul 4, 2022
@CommandMC CommandMC changed the title Feat/live log [General] Logging & launching improvements Jul 4, 2022
@flavioislima flavioislima requested review from Nocccer and arielj July 5, 2022 17:02
Nocccer
Nocccer requested changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

Have some suggestions and ideas we need to talk about.

electron/games.ts Outdated Show resolved Hide resolved
electron/gog/games.ts Outdated Show resolved Hide resolved
electron/legendary/games.ts Outdated Show resolved Hide resolved
electron/main.ts Outdated Show resolved Hide resolved
CommandMC and others added 4 commits July 6, 2022 21:20
Nocccer
Nocccer reviewed Jul 6, 2022
View reviewed changes
electron/main.ts Outdated Show resolved Hide resolved
Nocccer
Nocccer requested changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@Nocccer Nocccer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left some suggestions. For me it is important to have readable strings in the source code. A string with 1-2 linebreaks is maybe readable and therefore fine, but multine strings should be fast readable, especially with alot of linebreaks.

@Nocccer
Copy link
Collaborator

Nocccer commented Jul 7, 2022

Sorry for this discussion about how to represent multiline strings.
I would say if eslint puts everything in one line, don't use + or join.
If the string is pretty long (longer than the eslint rule) use + or join.
What do you think?

@CommandMC
Copy link
Collaborator Author

What do you think?

I honestly don't really mind. Whatever works works for me, if you want a specific style then I'll use that specific style

Nocccer
Nocccer approved these changes Jul 9, 2022
View reviewed changes
Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

Sorry for this back adn forth.
I think the best way is to write code always in mind someone else needs to understand it possible with the least effort. So readable code is the key.

But LGTM now.

@CommandMC CommandMC merged commit 1bc8871 into beta Jul 9, 2022
@CommandMC CommandMC deleted the feat/live-log branch July 9, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nocccer Nocccer Nocccer approved these changes

@arielj arielj Awaiting requested review from arielj

Assignees

@CommandMC CommandMC

Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CommandMC @Nocccer