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

feat: pass logger to integrations #7816

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Jul 26, 2023

Changes

I created two classes:

  • Logger, basic class. The objective of this class is to be used internally in our codebase. Ideally, we will pass around an instance of this class instead of passing logging. I plan to make this change gradually in the future.
  • AstroIntegrationLogger, similar to Logger, this class has the label stored inside of it. The main idea is that integrations should not know anything about labels.
    If an integration needs to do some logging with a different label - which I assume it's rare - they can use the fork function, which returns a new logger which has the same logging options.

Testing

Current tests should pass.

Docs

/cc @withastro/maintainers-docs for feedback!

@ematipico ematipico requested a review from a team as a code owner July 26, 2023 13:04
@changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2023

🦋 Changeset detected

Latest commit: ab3b54e

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release labels Jul 26, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Changelog and public API look good to me! Thanks @ematipico 💜

@matthewp
Copy link
Contributor

The diff is showing all changes unrelated to a logger. Like the capitalization change to API routes. Is that a mistake?

@ematipico ematipico force-pushed the feat/astro-logger-integrations-plt-690 branch from 6a4cd35 to ab3b54e Compare July 27, 2023 14:02
@ematipico
Copy link
Member Author

@matthewp now it should be fine

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I'm still somewhat on the side that we should only expose the logger in astro:config:done, but if the rest are fine with exposing it in more hooks, I'm fine with it too 👍

@ematipico ematipico merged commit 12402e7 into next Jul 28, 2023
@ematipico ematipico deleted the feat/astro-logger-integrations-plt-690 branch July 28, 2023 08:23
ematipico added a commit that referenced this pull request Jul 31, 2023
ematipico added a commit that referenced this pull request Aug 1, 2023
ematipico added a commit that referenced this pull request Aug 1, 2023
ematipico added a commit that referenced this pull request Aug 3, 2023
ematipico added a commit that referenced this pull request Aug 4, 2023
ematipico added a commit that referenced this pull request Aug 4, 2023
ematipico added a commit that referenced this pull request Aug 4, 2023
ematipico added a commit that referenced this pull request Aug 8, 2023
ematipico added a commit that referenced this pull request Aug 8, 2023
ematipico added a commit that referenced this pull request Aug 8, 2023
ematipico added a commit that referenced this pull request Aug 8, 2023
ematipico added a commit that referenced this pull request Aug 8, 2023
@jlarmstrongiv
Copy link

Thank you @ematipico ! I just updated my integration to use the new AstroIntegrationLogger in jlarmstrongiv/astro-i18n-aut@f7468c1 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants