Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[FEATURE] Support to only build certain dependencies #442
[FEATURE] Support to only build certain dependencies #442
Changes from 5 commits
6a94fc4
63f30e5
119fd55
6add4ea
a187584
7ac5b84
7fe851f
692d93b
e69b951
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's hard to understand from the code what the overall logic (idea) regarding priorities is. Please add a comment describing exactly this (what filter has priority over what other filter)
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.
I have changed the implementation. The new code has a lot more comments for describing priority.
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.
just out of curiosity: why does the builder still require include/exclude? We're already traversing the tree, looking at the new filtering options. Why can't we resolve the filters in the CLI layer and just give a list of projects (includes maybe) to the builder?
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.
For consumers like Kristian, who use the Node.js API of the builder directly.
I would try to keep the CLI layer as simple as possible and just pass the includes/excludes to the builder mostly untouched.
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.
Generics (<TypeParam, TypeParam...>), can only be used with the upper case Object, not with the primitive "object".
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.
Done
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.
I just had to revert to the lowercase style because the UI5 Tooling eslint rules doesn't allow the uppercase notation.
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.
Then please validate it locally:
npm run jsdoc
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.
Sorry, then the rule is wrong, disable it 😄
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.
I've updated my last comment with a smiley to indicate that I was not 100% serious, only 95%.
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.
Thank you! It left me quite puzzled 😵💫
In the UI5 Tooling JSDoc it seems that both work. The difference in the HTML is simply:
vs
In this case you're right that we indeed return an
Object
but it's also anobject
. From the rule: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.
Rule documentation IMO is only a second level of truth. I refer to https://jsdoc.app/tags-type.html and https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System .
But you're free to ignore me. I didn't "request changes", I only commented.
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.
Will do 😉
Let's keep it as-is for now. IMHO the documentation is good enough 🥉 . Changing the ESLint configuration across all projects is just too much work to make this (private) documentation perfect.
Related discussion in eslint-plugin-jsdoc: gajus/eslint-plugin-jsdoc#709
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.
Or add a single line disable. I'll leave that to @larskissel