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

Corral allows bad locators and doesn't error #118

Open
SeanTAllen opened this issue May 15, 2020 · 1 comment
Open

Corral allows bad locators and doesn't error #118

SeanTAllen opened this issue May 15, 2020 · 1 comment
Labels
bug Something isn't working help wanted Extra attention is needed needs discussion Needs to be discussed further

Comments

@SeanTAllen
Copy link
Member

I did an add with an incorrect locator and got:

{
"deps": [
{
"locator": "github.com/ponylang/peg",
"version": "0.1.0"
}
],
"info": {}
}

when I run corral fetch I get:

fetch: fetching from /home/sean/code/ponylang/changelog-tool
fetch: fetching dep: github.com/ponylang/peg @ 0.1.0
No dep bundle for: github.com/ponylang/peg

Which is horribly misleading given that the locator doesn't exist at all.

I propose this is addressed in two parts. Fix the bug of it not indicating the error on fetch.
Enhance to validate a locator on addition.

@SeanTAllen SeanTAllen added bug Something isn't working enhancement New feature or request help wanted Extra attention is needed labels May 15, 2020
@SeanTAllen SeanTAllen removed the enhancement New feature or request label May 18, 2020
@SeanTAllen SeanTAllen reopened this May 18, 2020
@SeanTAllen
Copy link
Member Author

SeanTAllen commented May 24, 2020

So the problem here is that the way that Corral determines the VCS to use work like this.

There's a primitive called VCSType that has the following to pick a VCS:

primitive VCSForType
  """
  This factory returns a VCS instance for any given VCS by name.
  """
  fun apply(env: Env, kind: String): VCS val ? =>
    match kind
    | "git" => GitVCS(env)?
    | "hg"  => HgVCS
    | "bzr" => BzrVCS
    | "svn" => SvnVCS
    else
      NoneVCS
    end

So if the VCS is unknown, you get the NoneVCS which NoOps everything. Thus it appearing to "work" in example I gave when opening this.

The match on string comes from vcs on the Deps class:

fun vcs(): String => locator.vcs_suffix.trim(1)

So a locator without a suffix will give you the NoneVCS and NoOps everything.

This seems like a design flaw in how Corral is determining the VCS to use and how the factory works.
Reasonably, there should be no NoneVCS outside of perhaps testing. It's existence is a smell.

At this point, I'm adding the "needs discussion" label to this issue. We need to rework how the VCS is determined and that requires some discussion.

@SeanTAllen SeanTAllen added the needs discussion Needs to be discussed further label May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed needs discussion Needs to be discussed further
Projects
None yet
Development

No branches or pull requests

1 participant