-
Notifications
You must be signed in to change notification settings - Fork 5
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
load user supplied plugins #487
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/prism-react-renderer@2.3.1, npm/react-dom@18.3.1, npm/react@18.3.1 |
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a typosquat?Package name is similar to other popular packages and may not be the package you want. Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
==========================================
+ Coverage 95.96% 96.29% +0.33%
==========================================
Files 20 20
Lines 1139 1243 +104
Branches 260 266 +6
==========================================
+ Hits 1093 1197 +104
Misses 45 45
Partials 1 1 ☔ View full report in Codecov by Sentry. |
e72e5d1
to
4431f73
Compare
OK. I think this is basically done. Next steps are:
Then I think you are good to 🚀 |
src/plugins.js
Outdated
// eslint-disable-next-line no-unused-vars | ||
parseDocument(contents, filename, format) { | ||
parseDocument(contents, filename, parser) { |
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.
A thought.
One of the open feature requests on this repo relates to multiple yaml documents embedded in a single file.
While this is generic enough to enable parsing additional/extended serialisation formats where there is a one-to-one mapping from file to document, is this flexible enough to allow someone to implement multi-doc yaml files or something like ndjson? I think no.
While it is not a breaking change, how could I modify this to enable that use-case?
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.
Another thought: In a world where multiple documents could live in one file, a Object<string, ValidationResult>
does not make sense for output. It should be an Array<ValidationResult>
(which implies a breaking change to the JSON output format).
Gah.
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.
..and then a third thought:
One option is to deal with this now. If you have to make a breaking release now, what else would you change in it?
Another option is to say: For the moment there is a one-to-one mapping between file and document and kick this can down the road. If you do that, how can you design the current interface now in such a way that the impact of that future change is minimal?
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.
OK. I think the most intuitive way to deal with this is:
- Change the hook name to
parseFile()
- Introduce some kind of
ParseResult
orDocument
type (which is just a thin wrapper object). The key thing here is we need to be able to easily tell how many of them we got back, which we can't really do with primitive types. The top-level construct of a document could be an array. - Allow
parseFile()
to return either a singleDocument
or an array ofDocument
objects
and then
- modify the internals to be able to deal with each document in turn
- modify the internals handle the fact that one filename can map to multiple documents
- update the JSON output format
- update the
results
value we pass togetAllResultsLogMessage()
(which will have knock-on effects to core plugins and the hypothetical sarif plugin) - update all the documentation
and then the next release will be breaking.
I reckon you might as well do this before anything depends on the plugin interface.
Wish I had thought of this earlier though 🙁
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.
There's actually a fair bit more nuance to this.
At the moment, log messages assume a 1-to-1 mapping between file and document. This applied to both internal messages to stderr and messages to stdout defined by getSingleResultLogMessage()
. So if you want to break that assumption it has a knock-on effect there too.
You also need a notation for documents within a file. I think multidoc.yaml[0]
, multidoc.yaml[1]
is probably fine for this.
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.
Here's another question to think about:
I don't really want to shave this entire yak right now. Is there a way you can implement a subset of changes now such that you
- don't have to do the whole thing right now but
- it allows you to do this later without making another BC break?
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.
OK. So I have pushed
which does the following things:
- rename
registerDocumentParsers
toregisterFileParsers
andparseDocument
toparseFile
- Change
Object<string, ValidationResult>
toArray<ValidationResult>
(Breaking change to JSON output format) - Add a
Document
object (but for now, the only option isparseFile
returns exactly one of these)
I thiiiiiink this does enough of the groundwork that I could allow parseFile()
to return an array of Document
objects in future and handle that internally without having to make another breaking change to the API surface.
Two things:
- Come back to this with a fresh perspective. Have you thought this through correctly? Is the code good? Have you missed anything?
- Given the next release must now be a major bump, is there anything else you now want to change that you were trying to avoid changing?
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
// eslint-disable-next-line no-unused-vars | ||
getSingleResultLogMessage(result, filename, format) { | ||
getSingleResultLogMessage(result, fileLocation, format) { |
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.
daca1a8
to
ba6c2e0
Compare
- make all docs packages devDependencies - only dependency review runtime packages
- registerDocumentParsers --> registerFileParsers - parseDocument --> parseFile
Object<string, ValidationResult> --> Array<ValidationResult>
- improve description for parser param - fix typos - improveme fileLocation description
ba6c2e0
to
071255b
Compare
File --> InputFile
4b97121
to
deb6622
Compare
OK. I am FINALLY happy with the plugin interface. It is: Input:
Output:
This has been such a vivid exercise in "naming things is hard" |
Closes #481
Closes #483
Closes #488