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

Feature/rules multi sources #340

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

myklemykle
Copy link

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.)

if (typeof source == 'function') {
source = source(name);

if (source instanceof Array) {
Copy link
Contributor

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?

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;
Copy link
Contributor

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) {
Copy link
Contributor

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') {
Copy link
Contributor

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?

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.

@mde
Copy link
Contributor

mde commented Jul 8, 2017

@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?

@CodeLenny
Copy link

CodeLenny commented May 10, 2018

@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
Copy link

@CodeLenny CodeLenny May 10, 2018

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?

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.

3 participants