-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
API for external applications #1860
Conversation
Now it passes but I can't understand. The diff is exactly the same @bep |
@@ -0,0 +1,28 @@ | |||
// Copyright 2015 The Hugo Authors. All rights reserved. |
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.
2016
Changed @bep |
|
||
func Run(flags []string) { | ||
commands.Execute(flags) | ||
} |
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.
One of my thoughts with an API was to limit it somehow to what the external tools needs. I know this is hard with the current setup, but if you limit it to the hugoCmd (for starters), you could name this func Build() or something. Then it also would be possible to create a more natural API than the CLI flags.
@bep Sure, you're right. Let's start with something simple, but it can be improved in the near future. The api could be also extended for the other commands, for example. Like Relatively to the flags, I'm thinking about it. Some kind of |
) | ||
|
||
func Build(flags []string) { | ||
commands.Execute(flags) |
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.
Wouldn't hugoCmd.ExecuteC() do the trick? Note the C at the end, then you can return an error to the client in case of errors. You would probably have to duplicate parts of commands.Execute -- but it would make a slightly better API.
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.
@bep that would be a good idea, but to do so I would have to export HugoCmd, wouldn't I?
I'm not entirely clear about the need for this or this approach. I think the reason for this is so that Hugo can be used more as a library by something like Caddy. Assuming that is the case then I think we should seriously consider taking one of two approaches. Approach 1: Cobra Command as APIWe already have an API of sorts implemented in the Cobra commands. Instead of redefining every command simply providing a wrapping mechanism that can easily call a command and pass in settings would be very useful. This approach wouldn't be Hugo specific but would be usable by any Cobra app. You've somewhat started this approach. Approach 2: An API packageThis would be a full API which would be used by Hugo Commands as well as any external libraries. We would move most of the code that are in the current commands into these API calls and the Commands package would just be a thin layer wrapping calls to these APIs. The approach in the current PR is a bit of both of these and I believe that's an incorrect approach. My gut is that the first approach may be the better one. It would be much simpler and much less disruptive of a change. There's an argument that we may want things exposed as an API that aren't currently exposed via a Command, but my gut feeling is that we would want to expose them at some point anyway. Thinking beyond this it could be a really powerful feature of Cobra if as part of the commands you would be defining simultaneously a CLI UI, an programatic API and possibly a REST API. |
My 50 cent:
|
Or we can export the commands. Or isn't that an option? |
Which is what we have now. If you export one command you export them all. |
49b4f8e
to
93e41a1
Compare
I think we need to restart this dicsussion. I will close this now. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
From #1859
New Website:
Build:
All of the stuff above was tested.