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.

Sign up for GitHub

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

Jump to bottom

fix(resolver): parentNames is missing in package warning breadcrumbs #4484

Merged
merged 1 commit into from
Oct 5, 2017
Merged

fix(resolver): parentNames is missing in package warning breadcrumbs #4484

merged 1 commit into from
Oct 5, 2017

Conversation

computnik
Copy link
Contributor

@computnik computnik commented Sep 16, 2017 •
edited by BYK
Loading

Summary

Bugfix for #4480. Change suggested by @BYK

Test plan
Running yarn on local.

Before Changes

warning [email protected]: Use uuid module instead

After Changes

warning raven > [email protected]: Use uuid module instead

@computnik computnik changed the title Fixed parentNames breadcrumbs in package warnings #4480 Fixed #4480 parentNames breadcrumbs in package warnings Sep 16, 2017
@computnik
Copy link
Contributor Author

computnik commented Sep 16, 2017 •
edited
Loading

Running Results

Before Change

LM-motnik:test motnik$ yarn install --force
yarn install v1.0.2
warning package.json: No license field
info No lockfile found.
warning [email protected]: No license field
[1/4] 🔍  Resolving packages...
warning [email protected]: Use uuid module instead
[2/4] 🚚  Fetching packages...
info [email protected]: The engine "node" is incompatible with this module. Expected version ">= 6.0.0".
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Rebuilding all packages...
success Saved lockfile.

After Change

LM-motnik:test motnik$ node ../lib/cli/index.js install --force
yarn install v1.0.2
warning package.json: No license field
info No lockfile found.
warning [email protected]: No license field
[1/4] 🔍  Resolving packages...
warning raven > [email protected]: Use uuid module instead
[2/4] 🚚  Fetching packages...
info [email protected]: The engine "node" is incompatible with this module. Expected version ">= 6.0.0".
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Rebuilding all packages...
success Saved lockfile.

@computnik computnik mentioned this pull request Sep 16, 2017
Closed
@BYK
Copy link
Member

BYK commented Sep 16, 2017

Looks good, thanks! Now we need to add tests so nobody can ever break this again!

@BYK BYK self-assigned this Sep 16, 2017
BYK
BYK requested changes Sep 16, 2017
View reviewed changes
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Needs tests. Otherwise looks good.

@BYK
Copy link
Member

BYK commented Sep 18, 2017

@motnik do you need help writing some tests for this?

@computnik
Copy link
Contributor Author

@BYK I am trying to figure out the tests structure. Would really appreciate some help in this regard.

@BYK
Copy link
Member

BYK commented Sep 21, 2017

@motnik sorry for the late response. So here are my suggestions:

@computnik
Copy link
Contributor Author

@BYK but these parentNames only come with warnings, when the packages are getting deprecated. Any ideas on how to go about that? is there a way to get a mock response like the one we get?

@BYK
Copy link
Member

BYK commented Sep 22, 2017

@BYK but these parentNames only come with warnings, when the packages are getting deprecated. Any ideas on how to go about that? is there a way to get a mock response like the one we get?

If you go with the second option, you should be able to create the request yourself and only check if parentNames is populated correctly instead of the warnings.

If you wanna test the actual warnings, then you need the first path with integration tests and set up a situation where you would get warnings. I'd got with the second option since it is simpler and more robust.

@BYK
Copy link
Member

BYK commented Oct 3, 2017

Ping!

@BYK
Copy link
Member

BYK commented Oct 3, 2017

@motnik need assistance? :)

@computnik
Copy link
Contributor Author

@BYK apologies for the delay. Just sent the test cases. Can you have a look ?

BYK
BYK approved these changes Oct 5, 2017
View reviewed changes
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Nice!

__tests__/package-request.js Show resolved Hide resolved
@BYK BYK changed the title Fixed #4480 parentNames breadcrumbs in package warnings fix(resolver): parentNames is missing in package warning breadcrumbs Oct 5, 2017
@BYK BYK merged commit 633b281 into yarnpkg:master Oct 5, 2017
BYK added a commit that referenced this pull request Oct 5, 2017
@BYK
chore(resolver): Minor improvements in resolver code and tests
7a9c134 
**Summary**

This is a follow up to #4484 and #4478 which improves the code
around those areas a bit and removes a now-unnecessary `while`
loop.

**Test plan**

Existing tests should pass.
@BYK BYK mentioned this pull request Oct 5, 2017
Merged
arcanis pushed a commit that referenced this pull request Oct 6, 2017
@BYK @arcanis
chore(resolver): Minor improvements in resolver code and tests (#4644)
9c2bbca 
* chore(resolver): Minor improvements in resolver code and tests

**Summary**

This is a follow up to #4484 and #4478 which improves the code
around those areas a bit and removes a now-unnecessary `while`
loop.

**Test plan**

Existing tests should pass.

* Fix logic
joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
@computnik @joaolucasl
fix(resolver): parentNames is missing in package warning breadcrumbs (
cf7b02c 
yarnpkg#4484)

**Summary**

Bugfix for yarnpkg#4480. Change suggested by @BYK 

**Test plan**
Running yarn on local.

_**Before Changes**_
```bash
warning [email protected]: Use uuid module instead
```

_**After Changes**_
```bash
warning raven > [email protected]: Use uuid module instead
```
joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
@BYK @joaolucasl
chore(resolver): Minor improvements in resolver code and tests (yarnp…
e982cf6 
…kg#4644)

* chore(resolver): Minor improvements in resolver code and tests

**Summary**

This is a follow up to yarnpkg#4484 and yarnpkg#4478 which improves the code
around those areas a bit and removes a now-unnecessary `while`
loop.

**Test plan**

Existing tests should pass.

* Fix logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BYK BYK BYK approved these changes

Assignees

@BYK BYK

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@computnik @BYK