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

Add Logger to library #1069

Merged
merged 14 commits into from
Dec 1, 2022
Merged

Add Logger to library #1069

merged 14 commits into from
Dec 1, 2022

Conversation

klenotiw
Copy link
Contributor

@klenotiw klenotiw commented Nov 22, 2022

Description

This PR moves the logger we had in shopify_app into the api library. This ensures that our logs are consistent across both repos.

How does it work?

Almost everything is the same but instead of storing the log level in ShopifyApp::Configuration, it's stored in ShopifyAPI::Context. So now if developers what to change the log level they would need to change it in their Shopify::Context.setup(...) call. Which lives in the same file as the shopify_app configurations. (config/intializers/shopify_app.rb)

I switched the Logger module to a class. This lets shopify_app create its own logger class and inherit its functionality, and override areas that deviate. For example there is no way to compare the version number in the shopify_app if the logger lives solely in the api. Also we should have a slightly different context method. Where the api would have "ShopifyAPI" as a prefix and the shopify_app would have "ShopifyApp". This will help developers know where exactly these logs are coming from.

I also made some error classes, FeatureDeprecatedError and LogLevelNotFound for the errors that we raise in methods found in Logger. Hopefully this gives some good hints on what is going on when the logger raises an error.

How has this been tested?

I also made a test file in test/logger_test.rb which does some unit tests on most of the logic found in the Logger class.

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

test/logger_test.rb Outdated Show resolved Hide resolved
lib/shopify_api/context.rb Outdated Show resolved Hide resolved
@klenotiw klenotiw requested a review from andyw8 November 23, 2022 15:06
Copy link
Contributor

@nelsonwittwer nelsonwittwer left a comment

Choose a reason for hiding this comment

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

Glad to see this functionality in the API! 🎉 🍻 Hopefully these changes don't take too long and make sense. If not, let me know and I'll be happy to help ya!

lib/shopify_api/context.rb Outdated Show resolved Hide resolved
lib/shopify_api/logger.rb Show resolved Hide resolved
lib/shopify_api/utils/session_utils.rb Outdated Show resolved Hide resolved
test/logger_test.rb Outdated Show resolved Hide resolved
test/logger_test.rb Outdated Show resolved Hide resolved
test/logger_test.rb Outdated Show resolved Hide resolved
lib/shopify_api/logger.rb Outdated Show resolved Hide resolved
lib/shopify_api/context.rb Outdated Show resolved Hide resolved
lib/shopify_api/context.rb Outdated Show resolved Hide resolved
lib/shopify_api/logger.rb Outdated Show resolved Hide resolved
Copy link

@cquemin cquemin left a comment

Choose a reason for hiding this comment

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

Some question around consistency of deprecated log message/configuration as I already mentioned on your other PR

lib/shopify_api/utils/session_utils.rb Outdated Show resolved Hide resolved
test/context_test.rb Show resolved Hide resolved
test/logger_test.rb Show resolved Hide resolved
lib/shopify_api/context.rb Show resolved Hide resolved
lib/shopify_api/context.rb Outdated Show resolved Hide resolved
lib/shopify_api/context.rb Show resolved Hide resolved
lib/shopify_api/logger.rb Show resolved Hide resolved
Copy link
Contributor Author

@klenotiw klenotiw left a comment

Choose a reason for hiding this comment

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

If I could approve I would. Thanks Nelson for the additions!

Copy link

@cquemin cquemin left a comment

Choose a reason for hiding this comment

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

Awesome work guys! This looks pretty good!

@nelsonwittwer nelsonwittwer merged commit 211245b into main Dec 1, 2022
@nelsonwittwer nelsonwittwer deleted the klenotiw/add-logger branch December 1, 2022 22:43
@klenotiw klenotiw mentioned this pull request Dec 2, 2022
5 tasks
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.

None yet

4 participants