Skip to content
This repository was archived by the owner on Apr 13, 2025. It is now read-only.

Add Log Prefix for logs in an instance #275

Conversation

ExtremTechniker
Copy link
Contributor

with custom logger
Move all necessary functions from nodecg.log to logger

with custom looger
Move all nessesaries nodecg.log to logger function
@ExtremTechniker
Copy link
Contributor Author

Closes #155

Copy link
Member

@hlxid hlxid left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. Please resolve the merge conflicts as described in the guide you got on discord. I can resolve the conflicts aswell, if you need help.

@TimTechDev
Copy link
Contributor

As you asked over on our discord how to resolve merge conflicts, I thought I could help you. So here we go:

Merge conflicts are a result of editing the same piece of code in two different branches, which you want to merge again. So the simplest solution is to merge the current https://github.com/codeoverflow-org/nodecg-io/master-branch into your feature-branch.

E.g., take a look at the diff of services/nodecg-io-twitch-api/extension/index.ts:

<<<<<<< HEAD (Current Change)
import { Result, emptySuccess, success, ServiceBundle, Logger } from "nodecg-io-core";
import { ApiClient } from "twitch";
=======
import { Result, emptySuccess, success, ServiceBundle } from "nodecg-io-core";
import { ApiClient } from "@twurple/api";
>>>>>>> upstream/master (Incoming Change)

You added the logger to the imports, while Daniel bumped the version of the Twitch API. So to resolve this, you would either add the logger to the Incoming Change or the updated the dependency in the Current Change.

Then push everything to your branch.

@TimTechDev
Copy link
Contributor

You may take a look at TimTechDev@987b4f5.

@ExtremTechniker
Copy link
Contributor Author

@daniel0611 Could you please resolve these merge conflicts, because I do not have write accesses to do this. Thanks!

@hlxid
Copy link
Member

hlxid commented Oct 30, 2021

Sure, no problem! You actually succesfully merged the master branch but there we other changes and thus more conflicts.

@TimTechDev
Copy link
Contributor

I want to mention that there are 4 test cases failing.

@hlxid
Copy link
Member

hlxid commented Oct 30, 2021

Yeah. These failures are because the logger is now passed to createClient etc. and the current test check for the passed arguments for the config. I'll fix them soon.
Wondering if these test failures were here before the merge. I can't remember.

@TimTechDev
Copy link
Contributor

TimTechDev commented Oct 30, 2021

Wondering if these test failures were here before the merge. I can't remember.

Yes, I think so checked on 8b823f3

Also, I looked at this commit history and want to mention that this will cause problems with git blame and other tools, because the contents of the master branch got duplicated.
edit:
This is also why there were merge conflicts, that were not resolvable by @ExtremTechniker.

@hlxid
Copy link
Member

hlxid commented Oct 30, 2021

What do you mean with "duplicated"? Because of 755ae35? GitHub shows the new changes from the other branch in merge commits that resolved conflicts. This is not the case when using git diff locally and blame is still working fine on this branch aswell: https://github.com/codeoverflow-org/nodecg-io/blame/9ea7a6b6d138e3960892828eb47cb41bfa4bf59f/services/nodecg-io-ahk/extension/index.ts
Anyways I'm gonna squash merge this PR for cleaner history becasue of the 4 fixup commits.

@hlxid hlxid merged commit 2b78f80 into codeoverflow-org:master Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants