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

RSDK-9117 - Remove logger from NewModuleFromArgs, and moduleName from ModularMain #4482

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

cheukt
Copy link
Member

@cheukt cheukt commented Oct 23, 2024

we want to force a breakage so that users will not use the wrong logger when making a Go module.

logs now look like

10/23/2024, 3:23:27 PM error acme:component:gizmo/gizmo1 mygizmo/mygizmo.go:61 hello err from the other side log_ts 2024-10-23T19:23:27.697Z

on app

when it used to be

10/23/2024, 3:23:27 PM error {moduleName}.acme:component:gizmo/gizmo1 mygizmo/mygizmo.go:61 hello err from the other side log_ts 2024-10-23T19:23:27.697Z

I think it's generally fine to no longer have the moduleName tacked on to the beginning of the logger name.

this is a breaking change but all the user needs to do is to get rid of the logger/module name from their function calls

cc: @benjirewis

@cheukt cheukt requested review from dgottlieb and randhid October 23, 2024 19:56
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Oct 23, 2024
module.ModularMain(
"complexmodule",
Copy link
Member

Choose a reason for hiding this comment

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

sighhhhhhhhh

Copy link
Member

Choose a reason for hiding this comment

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

@cheukt I'm fine with leaving ModularMain alone. And just leaving a small comment about why it takes in a string that isn't used.

Copy link
Member

Choose a reason for hiding this comment

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

no it's okay, I'm being slightly melodramatic here. We will update the go modules eventually.

Will anything break the binaries currently uploaded to the registry? from my understanding, no.

Copy link
Member Author

@cheukt cheukt Oct 23, 2024

Choose a reason for hiding this comment

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

no, it will not break any current binaries, this will only break if you update rdk on the package

@cheukt
Copy link
Member Author

cheukt commented Oct 23, 2024

we could also not break ModularMain, but we won't be using the moduleName parameter anymore. we could also further break NewLoggerFromArgs, but that seems unnecessary

@randhid
Copy link
Member

randhid commented Oct 23, 2024

we could also not break ModularMain, but we won't be using the moduleName parameter anymore.

I'm chaotic and okay with this.

break NewLoggerFromArgs, but that seems unnecessary

Is it used anywhere useful?

@cheukt
Copy link
Member Author

cheukt commented Oct 23, 2024

NewLoggerFromArgs is not used anywhere useful in rdk, but the parameter is technically still used if you do call NewLoggerFromArgs, which is why I'm keeping it for now

@cheukt cheukt merged commit 517e861 into viamrobotics:main Oct 25, 2024
18 checks passed
@cheukt cheukt deleted the break-module-from-args branch October 25, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants