-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
Removed old CLI #608
Removed old CLI #608
Conversation
Codecov Report
@@ Coverage Diff @@
## master #608 +/- ##
========================================
- Coverage 34.6% 34.6% -0.0%
========================================
Files 327 328 +1
Lines 10170 10174 +4
========================================
- Hits 3523 3521 -2
- Misses 6272 6279 +7
+ Partials 375 374 -1 |
pkg/drivers/container.go
Outdated
} | ||
} | ||
|
||
func (c *Container) HasDriver(name string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove Driver
word from Container's
methods. The Container
is in drivers
package. Thus when we write Container.Has
we know it means has driver
That's how Container
calls looks in ferret.go
:
// drivers
for _, drv := range i.drivers.RegisteredDrivers() { ... }
How will it look like after remove:
for _, drv := range i.drivers.Registered() { ... }
In my opinion, the second code looks better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to this way originally, but then I saw that Namespace
interface has *Function
, so to be consistent I decided to follow the pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be consistent because namespace
and function
are different entities. It's ok to write Namespace.RegisteredFunctions()
and Drivers.Registered
together
e2e/cli.go
Outdated
if err != nil { | ||
fmt.Println(err) | ||
os.Exit(1) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed because of os.Exit(1)
e2e/cli.go
Outdated
if err != nil { | ||
fmt.Println(err) | ||
os.Exit(1) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed because of os.Exit(1)
Closes #266