Bdymowski/oya tasks without run

[bart84ek]

This change is 

[bilus]

Reviewed 35 of 36 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @bart84ek)

---

oya_test.go, line 237 at r2 (raw file):

}

func (c *SuiteContext) CompileOya() {

Nice!

---

oya_test.go, line 248 at r2 (raw file):

		panic(err)
	}
	c.binDir = binDir

I think it would be more readable if you returned the bin dir from CompileOya.

c := SuiteContext{
  binDir: CompileOya(),
}

BTW. The function can be lower-case.

---

oya_test.go, line 272 at r2 (raw file):

	s.BeforeScenario(func(interface{}) { c.MustSetUp() })
	s.AfterScenario(func(interface{}, error) { c.MustTearDown() })
	// TODO after all remove c.binDir

Well? :)

---

cmd/root.go, line 39 at r2 (raw file):

var rootCmd = &cobra.Command{
	Use:   "oya",
	Short: "Oya is a task manager and runner",

Glad you took care of it. I think we need something more tool-oriented here but it'll do for now.

---

cmd/root.go, line 41 at r2 (raw file):

	Short: "Oya is a task manager and runner",
	Long:  "Oya takes the pain out of bootstrapping new deployable projects with packaged boilerplate & scripts.",
	// Uncomment the following line if your bare application

I think we can delete it.

---

cmd/run.go, line 77 at r2 (raw file):

		fmt.Println(err)
	}
	recurse := flagRecurse()

I believer this is incorrect. Hint: oya mytask --recurse (as user-defined flag) vs oya --recurse mytask.

I think it may be showing Cobra is not providing enough value anymore` you're trying to work around it.

---

cmd/run.go, line 89 at r2 (raw file):

		return err
	}
	p, err := project.Detect(workDir, installDir)

I know you mentioned it but it should never do the work twice (here & in the actual command) esp. with encryption, it would make everything SLOW.

Simple solution: change execTask so it takes Project and returns the function to run for command, thus creating a closure. (I'm assuming the Project here is the same as the Project there but the arguments are the same right?)

---

cmd/internal/get.go, line 23 at r2 (raw file):

	library, err := repo.Open(importPath)

	installDir, err := project.InstallDir()

It is a smell. The reason the function was where it was is that it references an env variable, aka applications outside interface. This is pretty much like arguments to oya. We want to keep it at the "outer" level and keep our inner packages testable.

If you want a separate package for "config", I'm fine with that, though I'd rather name a file in internal/ config.go rather than create a separate module because it too poorly limits the functions visibility and we may end up with the function unnecessarily used in other parts of the code instead of install dir being passed explicitly.

---

cmd/internal/render.go, line 55 at r2 (raw file):

	if found {
		if autoScope && scopePath == "" {
			scopePath, _ = project.LookupOyaScope()

Another smell.

Yet another responsibility added to the project package it doesn't really care about. It's not in any way related to what the package does.

Next thing: now project package reaches out to an environment variable, making it way harder to test overall.

This oya scope thing has only to do with the commands, nothing to do with inner code.

---

features/import.feature, line 25 at r2 (raw file):

    """
    Project: project
    Require:

The test should work without the Require. It's deliberately left out.

---

features/import.feature, line 34 at r2 (raw file):

      echo "check"
    """
  When I run "oya Oya.import github.com/tooploox/oya/next"

Just a thought: while we're at it, we could as well add an add command (oya add import so we have a place for more commands manipulating Oyafiles and limit the number of core targets). Non-blocking.

---

features/init.feature, line 7 at r2 (raw file):

Scenario: Init a project
  When I run "oya Oya.init"

I' leaning towards oya init but let's talk it through, forward-compatibility dictates that, in case a user-defined target hides a built-in target, there has to be a way to run the built in one. I think the solution is this:

1. User defines a render target.
2. It hides oya render built-in target.
3. When running oya render it emits a warning that says WARNING: user-defined targetrender` overwrites built-in target; rename target or invoke the built in target via "oya Oya.render".

We can still keep oya run for backwards compatibility.

I think it's very useful because we keep the concept of built-in packs BUT automatically alias the built-in targets into the global scope. Relatively easy to explain logically.

---

pkg/task/script.go, line 39 at r2 (raw file):

		switch err.(type) {
		case nil:
		// case interp.ExitStatus:

??

---

pkg/project/env.go, line 11 at r2 (raw file):

)

func InstallDir() (string, error) {

I hope I've convinced you this isn't the right place. :)

[bilus]

Reviewed 10 of 10 files at r3.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @bart84ek)