Skip to content

Conversation

@weakish
Copy link
Contributor

@weakish weakish commented May 3, 2016

No description provided.

@joshgoebel
Copy link
Contributor

Variable needs to be named "pip" or "package" everywhere. Should be consistent between check and installer.

@joshgoebel
Copy link
Contributor

Reason this can't use auto api?

@weakish
Copy link
Contributor Author

weakish commented May 6, 2016

Variable needs to be named "pip" or "package" everywhere. Should be consistent between check and installer.

I followed gem.rb, which used name

Some installers use package, and some use command name.
Which one is preferred?

Reason this can't use auto api?

I followed gem.rb.
I'll check auto_api later.

@weakish
Copy link
Contributor Author

weakish commented May 6, 2016

Reason this can't use auto api?

Does auto_api supports options?

          method="def #{method_name}(*names, &block)
            install #{class_name}.new(self, *names, &block)
          end"

https://github.com/sprinkle-tool/sprinkle/blob/master/lib/sprinkle/installers/package_installer.rb#L23

It seems that a lot of package installers do not support sudo.
What about extending auto_api to support options?

@joshgoebel
Copy link
Contributor

It seems that a lot of package installers do not support sudo.

Some installers are in different states than others, they probably all should properly support sudo.

What about extending auto_api to support options?

Do you have an idea of how? The problem is when you use *array that then taking other arguments becomes problematic.

This installer doesn't currently use options in any case so it seems like auto-api could be used as-is.


verify_api do
def has_pip(package)
@commands << "pip show #{package} | fgrep Name"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer grep -F and lets match 'Name:' with the colon to be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New commits pushed uses grep -F instead of fgrep,
with comments on matching pattern.

Also replace `fgrep` and `grep -F`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants