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

feat: provide the actual bad string in ValidateScheme #20

Conversation

pratik97
Copy link
Contributor

@pratik97 pratik97 commented Oct 13, 2019

Description

In ValidateScheme providing the actual scheme string against which we validate.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Related Issue

Fixes #10

Motivation and Context

How Has This Been Tested?

Added/Updated unit tests.

Screenshots (if appropriate):

Checklist:

  • I have updated the documentation (if required).
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I added a picture of a cute animal cause it's fun
    funnycat

@codecov-io
Copy link

codecov-io commented Oct 13, 2019

Codecov Report

Merging #20 into master will increase coverage by 1.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage    96.1%   97.22%   +1.11%     
==========================================
  Files           9       11       +2     
  Lines         154      180      +26     
  Branches       21       25       +4     
==========================================
+ Hits          148      175      +27     
+ Misses          6        5       -1
Impacted Files Coverage Δ
.../lockfile-lint-api/src/validators/ValidateHttps.js 100% <ø> (ø) ⬆️
...lockfile-lint-api/src/validators/ValidateScheme.js 94.73% <ø> (ø) ⬆️
...s/lockfile-lint-api/src/validators/ValidateHost.js 100% <ø> (ø) ⬆️
packages/lockfile-lint-api/src/common/constants.js 100% <0%> (ø)
...kages/lockfile-lint-api/src/common/ParsingError.js 100% <0%> (ø)
packages/lockfile-lint-api/src/ParseLockfile.js 100% <0%> (+2.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d07dde...c4f0e8a. Read the comment docs.

@lirantal lirantal self-requested a review October 18, 2019 22:13
@lirantal lirantal added bug Something isn't working hacktoberfest labels Oct 18, 2019
@lirantal
Copy link
Owner

@pratik97 a few issues here we should fix.

I think the output would be a bit hard to read, especially when we apply the same to hosts, etc.
With this PR it looks like this:

detected invalid scheme(s) https: for package: @babel/code-frame@^7.0.0
error: command failed with exit code 1

the invalid string in the lockfile is "http", which is wrongly showing here as "https" but regardless, it is too blended. How about we format it something like this as an output:

detected invalid scheme(s) for package: @babel/code-frame@^7.0.0
  -> http:
error: command failed with exit code 1

Do you think that's clear for a user?

The other issue is that the current code in this PR logs the allowed scheme, while we want to print out the bad one that is written in the lockfile and doesn't comply with the policy.

@lirantal
Copy link
Owner

another thought about how to print out the bad values is to maybe use an assertion style, something like this:

detected invalid scheme(s) for package: @babel/code-frame@^7.0.0
  expected: https:
  found: git+https
error: command failed with exit code 1

@lirantal
Copy link
Owner

@pratik97 great work here.

before we merge, can we:

  1. apply the same error formatting to the other flags too? I don't want to merge this in stages because then the errors will be inconsistent in their print-out.
  2. can you make the last line about the error error: command failed with exit code 1 to have a newline break?

@pratik97
Copy link
Contributor Author

pratik97 commented Nov 1, 2019

@lirantal I will work on it this weekend. Sorry, for the late reply :/

@lirantal
Copy link
Owner

lirantal commented Nov 1, 2019

@pratik97 sure

@lirantal
Copy link
Owner

lirantal commented Nov 2, 2019

@pratik97 thanks for pushing the commits, looks like you addressed all valdiators, or is there something still to be addressed in this PR?

@pratik97
Copy link
Contributor Author

pratik97 commented Nov 2, 2019

@lirantal no, I am done :)

@lirantal
Copy link
Owner

lirantal commented Nov 2, 2019

ok thanks, I shared on twitter a screenshot to get some feedback before we merge: https://twitter.com/liran_tal/status/1190655364758421507?s=20

One of which is to change found to actual which I agree with.

@lirantal
Copy link
Owner

lirantal commented Nov 3, 2019

@pratik97 let's add that change and I'll merge as a new major version.
we'll follow it up with other changes as they come by

@pratik97
Copy link
Contributor Author

pratik97 commented Nov 4, 2019

@lirantal done!

@lirantal lirantal merged commit 45fb7d2 into lirantal:master Nov 7, 2019
@lirantal
Copy link
Owner

lirantal commented Nov 7, 2019

Great stuff @pratik97, thanks for pushing this! ✨💪

@pratik97 pratik97 deleted the provide-actual-bad-string-in-validate-scheme branch November 7, 2019 06:52
@lirantal
Copy link
Owner

lirantal commented Feb 1, 2020

fixes #10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI option to report all non-whitelisted URLs used
3 participants