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

Use skipIntegrityCheck instead of skipIntegrity in licenses command #3239

Merged
merged 1 commit into from
Apr 24, 2017

Conversation

voxsim
Copy link
Contributor

@voxsim voxsim commented Apr 23, 2017

In #3238 I noticed that we changed the skipIntegrity flag in skipIntegrityCheck (this is the commit bd8e326)

Test plan
I have no idea how to test it; right now I don't have time to see this part. Let's park here for now.

@voxsim voxsim changed the title use skipIntegrityCheck instead of skipIntegrity Use skipIntegrityCheck instead of skipIntegrity in licenses command Apr 23, 2017
@bestander bestander merged commit cd4a8d7 into yarnpkg:master Apr 24, 2017
@bestander
Copy link
Member

No need for a unit test, I think this should have been caught by Flow.

@voxsim
Copy link
Contributor Author

voxsim commented Apr 24, 2017

I thought the same, we should thank git bisect to found the commit :D

I think I will spend some time to understand package-* and integrity-checker because IMO they require some love

@bestander
Copy link
Member

bestander commented Apr 24, 2017

Yeah.
I am planning to do a bit more changes to integrity-checker in the next couple of weeks.

  • remove reporter.warn from it
  • split check operation from reading artifacts data from .yarn-integrity
  • move integrity check into an earlier phase of install command

Happy to collaborate and sync on this

@voxsim
Copy link
Contributor Author

voxsim commented Apr 24, 2017

That sounds great :D

@voxsim
Copy link
Contributor Author

voxsim commented Apr 24, 2017

I think to add some unit/integration testing do understand what effectively does

@voxsim
Copy link
Contributor Author

voxsim commented Apr 24, 2017

Ah before I forget, If you want to delegate something, I am happy to collaborato too :D

@bestander
Copy link
Member

Would you want to extract the reporter out of integrity-checker?
I think I won't be able to get down to this task before next week.

@voxsim
Copy link
Contributor Author

voxsim commented Apr 24, 2017

Sure, I will open a pull request soon. By 'extract the reporter out of integrity-checker' do you mean remove every report.warn from integrity-checker and add a generic this.reporter.warn(this.reporter.lang('integrityFailed')); or something else?
One other guess could be extract _compareIntegrityFiles out from integrity-checker O.o but IMO does not seem a great idea

@bestander
Copy link
Member

bestander commented Apr 24, 2017

I think integrity-checker should not have side effects and reporting warning is one.
Separation of concerns.
We actually care about outputting warnings about what exactly failed in integrity check in check command.

Would be great if those reporter.warn could be moved to check.js command and integrity-checker would just output whether integrity matches and if not in what aspect of check has failed (e.g. FLAGS_DONT_MATCH, FILES_DONT_MATCH etc).

This was referenced Apr 28, 2017
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.

2 participants