Skip to content

Conversation

@ShyamSunder149
Copy link

@ShyamSunder149 ShyamSunder149 commented Jan 7, 2026

Hey,

I have addressed issue #2 and #4 in this PR. Kindly check and let me know whether this can be merged..

Note : Tried running lint but since there were a lot of blockers in the previous code it will not run as expected but my code doesn't have blockers. just mentioning this if this will be run in a pipeline...

Thanks

Copy link
Member

@ritvikos ritvikos left a comment

Choose a reason for hiding this comment

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

For #2, the implementation adds a restrictive logger abstraction (slogWrapper) for components. Actually it deviates from the intent of exposing slog.Logger directly at component-level (here Spooler)

For #4, the implementation adds deep nesting. It'd be cleaner to expose relevant methods directly in Writer.

Thanks for your time :)

@ritvikos ritvikos force-pushed the main branch 4 times, most recently from 83dcc79 to 545f351 Compare January 12, 2026 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants