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

Look for Sakefile in parent folders as well #23

Closed
wants to merge 6 commits into from

Conversation

wbiker
Copy link

@wbiker wbiker commented Jul 10, 2018

My idea was to be able to have not just one Sakefile in the current folder, instead Sakefiles in parent folders should be find as well. That has the benefit, that tasks can be available or overwritten depending of the current working folder.

For that, I added a method to the pm file plus a test file to test it.
Changed the behavior though. Sake no longer dies if a task already exists. It just ignores the task found in parent folders with the same name. So, always the task is used that is the nearest to the current working dir. That might be not the desired approach for other users.

@coke
Copy link
Contributor

coke commented Jul 10, 2018

Is there prior art on this for any other build tools?

This seems like it could end up surprising the user.

@wbiker
Copy link
Author

wbiker commented Jul 10, 2018

Good point.
I was not sure to PR this. But I thought other might be interested in this as well.
So, I am open for discussions

@AlexDaniel
Copy link
Member

AlexDaniel commented Jul 10, 2018

Is there prior art on this for any other build tools?

Rake does it.

Rakefile:

task :default do
  puts 'default'
end

Trying it out:

alex@librepad:~/raketest/foo$ rake

Output:

(in /home/alex/raketest)
hello

@AlexDaniel
Copy link
Member

@wbiker maybe it's a good idea to look how Rake does it. For example, maybe there are some edge cases that need to be covered? Otherwise looks good to me.

@coke
Copy link
Contributor

coke commented Jul 11, 2018

-0.5 from me.

@wbiker
Copy link
Author

wbiker commented Jul 11, 2018

I dent to close this PR. Sake is around quite a while and I have no idea what side effects could be shown if it is merged.
Or, should I introduce a command line parameter to enable this code? So, it could be behave like before as default.

@AlexDaniel
Copy link
Member

AlexDaniel commented Jul 11, 2018

Sake is around quite a while and I have no idea what side effects could be shown if it is merged.

Now is probably the best time to make a change like this.

However, the PR has to be modified I think. One problem, as far as I can see, is that it does not chdir into the directory where the sakefile file is located. That can be a real issue because most Sakefiles will probably expect a bunch of paths to exist, so the functionality shouldn't be “execute any Sakefile in the current directory” but “go back a little if I'm in a subdirectory and run a Sakefile there”.

Also, currently it evals all of the Sakefiles up to / (if I'm not mistaken, just looking at the code), and that's most likely incorrect. If at any time I'd want to EVAL an upper Sakefile, I'd just start the new one with EVALFILE ‘../Sakefile’ or something like that. So this has to be fixed also.

@wbiker
Copy link
Author

wbiker commented Jul 12, 2018

@AlexDaniel ...it does not chdir into the directory where the sakefile file is located...
My idea was to overwriting tasks in parents folders, not to run them there. So I can have a task 'test', { prove dadada } in /repos/Sakefile and this task is invoked in any subfolder in /repos. Except there is a Sakefile in one of the subfolders with another task 'test' { prove -Iwhatever dadada }, then sake test makes something different.
THat is the idea behind my changes. But I see your point.
What about task attributes. Something like:
task 'test', OVERWRITE, { sdsd }

@wbiker
Copy link
Author

wbiker commented Jul 15, 2018

Since this issue is more than just looking for sakefiles in the path, I close this.
Nevertheless, I would like work on this because I think it would be useful. Only in this form it is probably useful for me.

@wbiker wbiker closed this Jul 15, 2018
@AlexDaniel
Copy link
Member

@wbiker Maybe consider making a module that depends on and uses Sake? This way you can have let's say resake command that uses Sake, so you get the best out of Sake yet it will do what you want it to do.

@wbiker
Copy link
Author

wbiker commented Jul 15, 2018

Thanks. Will stick with my changed code for now. But will think about that

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.

None yet

3 participants