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

Internal logging package and a test-utils package to help them #182

Merged
merged 17 commits into from
Oct 23, 2019

Conversation

ajaymathur
Copy link
Contributor

@ajaymathur ajaymathur commented Oct 5, 2019

re: #142

Adding two packages, @changesets/logger and @changesets/test-utils.

@changesets/logger

Log messages to stdout.

Usage:

import { warn } from "@changesets/logger";

warn('message part 1', 'message part 2');

@changesets/test-utils

All test utilities, at the moment only one - temporarilySilenceLogs, it will silence all logs done in the changesets file. **It will not highjack console, therefore console.*(xxx) will still. Which in my perspective is needed to not accidentally silence the error message.

// In tests
import { temporarilySilenceLogs } from "@changesets/test-utils";

temporarilySilenceLogs();
  • Add a logger
    • add package.
    • add tests and documentation.
    • change usage in the cli package.
    • Method to silent the internal logging in tests.
    • Document this internal logging.

This PR is in progress, but inputs at any step are welcome.

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2019

🦋 Changeset is good to go

Latest commit: 86892f9

We got this.

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

@ajaymathur ajaymathur force-pushed the issue/142-logger branch 3 times, most recently from adfbb84 to 36a6eb9 Compare October 19, 2019 23:14
@ajaymathur ajaymathur changed the title [WIP] Introducing logger package Internal logging package and a test-utils package to help them Oct 19, 2019
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks for this!!

packages/logger/README.md Outdated Show resolved Hide resolved
packages/logger/README.md Outdated Show resolved Hide resolved

**success**: Use `success` to assert to users that their instructions have completed succesfully.

**warn**: Use `warn` to print warning messages, something that user could action on now or later without much impact of their work.
Copy link
Member

Choose a reason for hiding this comment

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

I really like this, talking about it when to use them is great!

packages/logger/README.md Outdated Show resolved Hide resolved
packages/logger/README.md Outdated Show resolved Hide resolved
packages/test-utils/README.md Outdated Show resolved Hide resolved
packages/test-utils/src/index.ts Outdated Show resolved Hide resolved
@emmatown
Copy link
Member

emmatown commented Oct 20, 2019

Also: needs a changeset/changesets

ajaymathur and others added 7 commits October 20, 2019 13:10
Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>
Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>
Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>
Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>
Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>
Co-Authored-By: Mitchell Hamilton <mitchell@hamil.town>
@ajaymathur
Copy link
Contributor Author

The commit history is not pretty at the moment. I feel some of the messages can be improved and some commits be squashed. I am spend time in fixing the messages. If we do a merge. Else we can update message on squash and commit. 🙂

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Could you split the changeset into two, one for logger and test-utils that says initial release and another for cli that says it's using @changesets/logger? Apart from that, LGTM. re commits: we'll squash merge.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

@ajaymathur
Copy link
Contributor Author

ajaymathur commented Oct 22, 2019

Hey @mitchellhamilton, Thanks for reviewing the PR. Should we merge it?

@emmatown
Copy link
Member

I was going to wait for @Noviny to ✅ it but it's not really a change that's likely to cause problems so let's merge it.

@emmatown emmatown merged commit 51a0d76 into master Oct 23, 2019
@emmatown emmatown deleted the issue/142-logger branch October 23, 2019 00:08
@github-actions github-actions bot mentioned this pull request Oct 23, 2019
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

2 participants