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

New modules logger, errfmt and environment #4113

Closed
wants to merge 6 commits into from

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Jun 10, 2024

Part of #4111 solution.

1. Explain what the PR does

1122db1 chore: use environment module
6ee5a54 chore: mv pkg/utils/environment into environment mod
4e7c7de chore: use errfmt module
efae8e6 chore: move pkg/errfmt into errfmt module
0e22c48 chore: use logger module
c211422 chore: move pkg/logger into logger module

2. Explain how to test it

3. Other comments

Well, there's no straight way of doing this relocation. So, after this merge will have at least 2 more PRs to point to the right modules hashes.

Copy link
Contributor

@rscampos rscampos left a comment

Choose a reason for hiding this comment

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

LGTM

@geyslan
Copy link
Member Author

geyslan commented Jun 11, 2024

@AlonZivony do you think this would help with #4111 matter? I ask since I know this will increase maintenance efforts for both ends. Decoupling in modules can be a start to move them to repos if the respective codebase demands it anyway.

@yanivagman
Copy link
Collaborator

yanivagman commented Jun 13, 2024

A few thoughts:

  1. Should we have so many modules? Maybe we can converge some?
  2. Should they be under the root of the tree? Can't we put them under pkg?
  3. Do we really want errfmt as a module (it doesn't have much logic in it).

Maybe we should remove the error handling from within the environment package and let the caller handle any returned errors?

@geyslan
Copy link
Member Author

geyslan commented Jun 13, 2024

A few thoughts:

1. Should we have so many modules? Maybe we can converge some?

We should not have. I agree that converging them is the best approach to us. But we have to have in mind that splitting should avoid cyclic import issues.

2. Should they be under the root of the tree? Can't we put them under pkg?

If modules, bring them up to avoid cyclic import issues.

3. Do we really want errfmt as a module (it doesn't have much logic in it).

No, but it can be part of a converged module, like utils - I know, I know, utils is the worse naming option, but since we would converge different pkgs into it, they could be imported like utils/errfmt, utils/environment...

Maybe we should remove the error handling from within the environment package and let the caller handle any returned errors?

Well, this is an option too.

@geyslan
Copy link
Member Author

geyslan commented Jun 17, 2024

We are choosing another approach. #4129

@geyslan geyslan closed this Jun 17, 2024
@geyslan geyslan deleted the new-modules branch June 28, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants