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

Maintenance: refactor Logger types and interfaces to improve consistency and clarity #1729

Closed
1 of 2 tasks
dreamorosi opened this issue Oct 2, 2023 · 4 comments · Fixed by #1758
Closed
1 of 2 tasks
Assignees
Labels
completed This item is complete and has been merged/shipped good-first-issue Something that is suitable for those who want to start contributing logger This item relates to the Logger Utility
Milestone

Comments

@dreamorosi
Copy link
Contributor

Summary

Review all type and interface definitions and usage throughout the codebase to identify opportunities for refactoring and clean up. This will help improve consistency in naming, structure, and usage of types across the project.

Specific areas to investigate:

  • Inconsistent naming (plural vs singular, capitalization, abbreviations, etc.)
  • Misnamed types that are not actually types but constants or enums
  • Usage of any where more specific types could be defined
  • Opportunities to convert usage of enums to as const types
  • Inheritance hierarchies that could be simplified
  • Re-organization of types into more logical modules/namespaces

Why is this needed?

As the project has grown organically, inconsistencies in how types are defined and used have emerged. By refactoring and cleaning these up, we can improve maintainability, clarity, and reliability of the codebase.

More consistent and intention-revealing names will also improve onboarding and comprehension as well as DX.

Converting usage to proper types where applicable will enable stronger compiler checking and editor tooling support. Overall, this refactoring will leave the code in better shape for continued development and maintenance.

Which area does this relate to?

No response

Solution

Examples of types/interfaces/objects that could be improved:

  • ClassThatLogs (here) might be converted to an interface, renamed, and made to include all methods
  • ConstructorOptions (here) could be renamed to something more explanatory like LoggerOptions or similar (check other utilities)
  • LogJsonIndent (here) should be moved out of this file and into a constants.ts file similar to what done in other utilities, also it should be converted to an object as we are moving away from enums.

Important

Before starting to work on a PR for this issue, the assignee should do an investigation of the code and propose an action plan under this issue.

Once the plan has been discussed and agreed upon with at least one maintainer work on the PR can start.

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@dreamorosi dreamorosi added good-first-issue Something that is suitable for those who want to start contributing help-wanted We would really appreciate some support from community for this one logger This item relates to the Logger Utility discussing The issue needs to be discussed, elaborated, or refined labels Oct 2, 2023
@dreamorosi dreamorosi added this to the Version 2.0 milestone Oct 2, 2023
@codyfrisch
Copy link

Another task, though simple, is to relocate type HandlerMethodDecorator to commonso/utils/lambda. It appears to be duplicated in the core modules

This has the added benefit that we can add commons as a dev dependency for modules that will implement handler decorators but don't need anything from logger (though obviously as dev dependency and declaring the import as a type it doesn't matter a whole lot). Just seems cleaner to me.

@dreamorosi
Copy link
Contributor Author

Hi @codyfrisch, you're right and making this change makes sense.

We have already moved that type in the v2 branch and switched to using it in some of the utilities.

I haven't switched Logger to use it yet because I wanted to see if #1744 would be merged first, but I'm planning to.

@dreamorosi dreamorosi removed help-wanted We would really appreciate some support from community for this one hacktoberfest labels Oct 20, 2023
@dreamorosi dreamorosi self-assigned this Oct 20, 2023
@dreamorosi dreamorosi linked a pull request Oct 20, 2023 that will close this issue
9 tasks
@github-actions github-actions bot added pending-release This item has been merged and will be released soon and removed discussing The issue needs to be discussed, elaborated, or refined labels Oct 20, 2023
@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels Nov 1, 2023
@aws-powertools aws-powertools deleted a comment from github-actions bot Nov 2, 2023
@dreamorosi dreamorosi added pending-release This item has been merged and will be released soon completed This item is complete and has been merged/shipped and removed completed This item is complete and has been merged/shipped pending-release This item has been merged and will be released soon labels Nov 2, 2023
@dreamorosi
Copy link
Contributor Author

This is available in preview starting from the 2.0.0-alpha.1 release. You can install this version using the next tag, i.e. npm i @aws-lambda-powertools/logger@next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped good-first-issue Something that is suitable for those who want to start contributing logger This item relates to the Logger Utility
Projects
Development

Successfully merging a pull request may close this issue.

2 participants