-
Notifications
You must be signed in to change notification settings - Fork 191
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/rules multi sources #340
base: main
Are you sure you want to change the base?
Conversation
if (typeof source == 'function') { | ||
source = source(name); | ||
|
||
if (source instanceof Array) { |
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.
Looks like duplcate logic of what's below. Can you refactor this so that it does its function stuff first, then do the regular logic for Array/String?
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 had a similar thought - but then noticed each instance works slightly differently.
The if string
outside of if function
uses the matcher:
src = completeNamespace( Matcher.getSource(name, pattern, source) )
while if string
inside of if function
just uses completeNamespace
:
src = completeNamespace(source);
(same for if array
)
From skimming the source, I think that means if a string is passed directly, it can contain wildcards, but if it's returned from a function, it must be the entire filename of the source, without wildcards, etc.
Now, maybe wildcards should always be applied (or I've misinterpreted the code I skimmed), but that's my 2 cents.
|
||
else { | ||
// It's a function that returns a headache. | ||
debugger; |
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.
Remove debugger statement.
src = source(name); | ||
|
||
// 2: An array of sources | ||
else if (source instanceof Array) { |
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.
instanceof
is not a reliable check for Array. Prefer Array.isArray
.
|
||
// Deal with the various things the source argument can be: | ||
// 1: A function | ||
if (typeof source == 'function') { |
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'm not completely sold on the function idea, mostly because part of the idea here was to make Rules behave more like Tasks. Could you leave this change for a different PR, and maybe implement it for both Tasks and Rules?
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.
@mde I think that this is just a more explicit version of the else {
that was already in the codebase (https://github.com/jakejs/jake/pull/340/files/afe002d343308d61527ec894e78630889f406281#diff-03fda114c6cfeff1993dbc9c03d7724bL255) (exact body) - just more explicit about the conditions.
@myklemykle This is a good start -- I've added some comments. Could we pull the function-as-prereq into a different PR that implements it across the board? |
@myklemykle @mde I'm interested in using this feature myself - actually, I assumed the feature was included in Jake, and was scratching my head for a while when it didn't work. What's the status of this MR? I would be willing to attack the remaining comments if needed - making "function-as-prereq" work in all cases might be a little too much work for me right now, but I can definitely take care of the other items. |
|
||
// Generate the prerequisite for the matching task. | ||
// It is the original prerequisites plus the prerequisite | ||
// It is the original prerequisites plus the definied prerequisite/prerequisites |
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.
definied
If changes are being made, might as well fix a typo in the new code?
With this change, the second argument to rule() can be an array of source file names. That makes rules more powerful, and more orthogonal to how fileTasks already specify their requirements.
This change also extends the functionality when the second argument is a function; that function may return such an array of sources, instead of just a single source. (This is particularly handy for my work, and also seems orthogonal to how rules & fileTasks work generally.)