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

Port logging utils #43

Merged
merged 11 commits into from
Oct 6, 2023
Merged

Port logging utils #43

merged 11 commits into from
Oct 6, 2023

Conversation

kemmerle
Copy link
Contributor

@kemmerle kemmerle commented Oct 4, 2023

Description and Context

As part of our final push to port files from the cli-lib to the local-dev-lib library, I'm porting some logging utils that cannot be copied directly to hubspot-cli. This is because they're used in multiple packages:

  • logs.ts: The one exported function in this file, outputLogs, is called in both the cli and serverless-dev-runtime packages in hubspot-cli.

  • processFields.ts: processFields is only called in handleFieldsJs.ts. handleFieldsJs is called in both the uploadFolder.ts and watch.ts files, which are also used in the VSCode extension.

  • checkAndWarnGitInclusion (in git.ts): is called in both the cli and webpack-cms-plugins packages in hubspot-cli.

Testing

I would really appreciate if someone would go through the processFieldsJs file in particular to see whether the changes I made from the original file in cli-lib make sense. I've made notes on particularly tricky bits of code.

TODO

  • Address feedback

Who to Notify

@camden11 @brandenrodgers

lib/cms/processFieldsJs.ts Outdated Show resolved Hide resolved
utils/loggingUtils/logger.ts Outdated Show resolved Hide resolved
utils/loggingUtils/logs.ts Outdated Show resolved Hide resolved
lib/cms/handleFieldsJS.ts Outdated Show resolved Hide resolved
lib/cms/processFieldsJs.ts Show resolved Hide resolved
lib/cms/processFieldsJs.ts Outdated Show resolved Hide resolved
utils/loggingUtils/logger.ts Outdated Show resolved Hide resolved
utils/loggingUtils/logger.ts Outdated Show resolved Hide resolved
utils/loggingUtils/logger.ts Outdated Show resolved Hide resolved
utils/loggingUtils/logs.ts Outdated Show resolved Hide resolved
lib/loggingUtils/git.ts Outdated Show resolved Hide resolved
lang/en.lyaml Outdated Show resolved Hide resolved
Copy link
Contributor

@brandenrodgers brandenrodgers left a comment

Choose a reason for hiding this comment

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

One very small nit about vertical spacing in the git warning message, but otherwise lgtm!

@kemmerle kemmerle merged commit 15d63bb into main Oct 6, 2023
1 check passed
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.

3 participants