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

Improve logging for mods #445

Merged
merged 51 commits into from
Jul 7, 2023
Merged

Conversation

ASpoonPlaysGames
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames commented Apr 7, 2023

  • Removes the log spam of "changing mod search path"
  • Logs mods as they are loaded, alongside their ConVars, Scripts, ConCommands, etc.
  • Warns in the console about errors in the mod.json instead of silently skipping things

@GeckoEidechse
Copy link
Member

Regarding:

  • Removes the log spam of "changing mod search path"

Do we still keep an indicator from where the mod is loaded from? Not saying we need to keep that message but given that with auto-download ( #362 ) we'll have a second location where we are loading mods from it would probably be useful to know still where the mod is loaded from ^^

@ASpoonPlaysGames
Copy link
Contributor Author

Do we still keep an indicator from where the mod is loaded from?

Unsure, but I can always add it when the mod is loaded

@pg9182 pg9182 added the waiting on changes by author Waiting on PR author to implement the suggested changes label Apr 18, 2023
@ASpoonPlaysGames
Copy link
Contributor Author

@F1F7Y 's PR to this branch moved things over to their own functions

I have now fixed merge conflicts (which meant parsing InitScript) so I would consider this complete now I think

@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code and removed waiting on changes by author Waiting on PR author to implement the suggested changes labels May 1, 2023
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Confirmed compiling and working in testing.
Meaningful logs will help a lot in tickets.

@Alystrasz Alystrasz requested review from GeckoEidechse and F1F7Y June 10, 2023 14:53
@ASpoonPlaysGames
Copy link
Contributor Author

image
fifty contributed to this PR, so probably not best if they review

@Alystrasz
Copy link
Contributor

Good catch!

@Alystrasz Alystrasz removed the request for review from F1F7Y June 10, 2023 15:40
@Alystrasz Alystrasz added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs testing Changes from the PR still need to be tested labels Jun 10, 2023
@F1F7Y
Copy link
Member

F1F7Y commented Jun 22, 2023

I think i can improve on this a bit more, but think its good to be merged as is for now

@GeckoEidechse
Copy link
Member

Imma try to update branch to CMake, hopefully PR survives the change ^^

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Tested and confirmed:

  • Removes the log spam of "changing mod search path"
  • [sorta] Logs mods as they are loaded, alongside their ConVars, Scripts, ConCommands (didn't have any example, someone gimme one please <3), etc.
  • Warns in the console about errors in the mod.json instead of silently skipping things

Honestly, if other contributors/modders want this fine by me. Some stuff is nice, kinda indifferent about the rest.

@uniboi
Copy link
Contributor

uniboi commented Jun 29, 2023

just merge it istg

@Alystrasz Alystrasz removed the needs code review Changes from PR still need to be reviewed in code label Jul 2, 2023
@GeckoEidechse GeckoEidechse merged commit 9f9e3a9 into R2Northstar:main Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants