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

[chore] Small updates #133

Merged
merged 9 commits into from
Feb 19, 2025
Merged

[chore] Small updates #133

merged 9 commits into from
Feb 19, 2025

Conversation

yoannmoinet
Copy link
Member

@yoannmoinet yoannmoinet commented Feb 17, 2025

What and why?

Some small updates to lighten #134 and #137

How?

  • Change how we use the logger to simplify the documentation and reduce complexity of implementation. TLDR; Add getLogger on the GlobalContext.
  • Move the injections state to its own plugin, for more simplicity and self contenance.
  • Factorise some readmes types.

@yoannmoinet yoannmoinet marked this pull request as ready for review February 18, 2025 11:00
@yoannmoinet yoannmoinet requested a review from a team as a code owner February 18, 2025 11:00
@yoannmoinet yoannmoinet requested review from ruimartin, elbywan and Ayc0 and removed request for a team and ruimartin February 18, 2025 11:00
@yoannmoinet yoannmoinet enabled auto-merge (rebase) February 19, 2025 10:28
getLogger: getLoggerFactory(build, options.logLevel),
// This will be updated in the injection plugin on initialization.
inject: () => {
throw new Error('Inject function called before it was initialized.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

question I'm not very familiar with the code but is that error caught and reported properly elsewhere or could it just break the build?

@@ -25,11 +23,21 @@ const logPriority: Record<LogLevel, number> = {
none: 4,
};

// Exported for testing.
// Which separator to use for plugin names.
export const NAME_SEP = '>';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
export const NAME_SEP = '>';
export const PLUGINS_NAME_SEP = '>';

// Which separator to use for plugin names.
export const NAME_SEP = '>';

const cleanName = (name: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
const cleanName = (name: string) => {
const cleanPluginsName = (name: string) => {

@yoannmoinet yoannmoinet merged commit 4ede575 into master Feb 19, 2025
4 checks passed
@yoannmoinet yoannmoinet deleted the yoann/small-updates branch February 19, 2025 10:31
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