-
-
Notifications
You must be signed in to change notification settings - Fork 627
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: refactor info package #1382
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need improve tests
@rishabh3112 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @evilebottnawi Please review the new changes. |
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide example of current output? I think we should migrate on snapshots in future
No, it is good idea, we should remove/mock platform specific output from snapshot (like do it webpack) |
@evilebottnawi |
Now I have to spar every time what the output looks like, it’s extremely inconvenient and I can skip some regressions, run it locally every time and look very time-consuming, so we need to simplify this |
@rishabh3112 I will improve tests in future and you see how it is better, let's avoid it here, just provide example current default output |
Here it is #1382 (comment) |
@evilebottnawi also should I add the |
Let's rename
In the next PR |
@evilebottnawi anything else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I think it should be merged. @evilebottnawi @rishabh3112 @anshumanv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, thanks!
Thanks, feel free to integrate env package into CLI |
What kind of change does this PR introduce?
refactor/feature.
Did you add tests for your changes?
WIP
If relevant, did you update the documentation?
WIP
Summary
Would be adding info package as direct dependency as per roadmap
Does this PR introduce a breaking change?
No
Other information
No