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

make commands 'context' (directory) aware #885

Open
andyberry88 opened this Issue Aug 7, 2014 · 13 comments

Comments

Projects
None yet
4 participants
@andyberry88
Member

andyberry88 commented Aug 7, 2014

Change each command so it can take an optional 'context' directory which specifies the directory being acted in. For example brjs create-blade <app> <bladeset> <blade> becomes brjs create-blade myBlade [path/to/bladeset]. We should also check whether this then allows commands to be run from any directory.

@andyberry88 andyberry88 added this to the 0.13 milestone Aug 7, 2014

@leggetter leggetter referenced this issue Aug 7, 2014

Closed

brjs command global install #1

4 of 5 tasks complete

@andyberry88 andyberry88 removed this from the 0.13 milestone Aug 29, 2014

@andyberry88 andyberry88 added this to the 0.14 milestone Oct 3, 2014

@andyberry88 andyberry88 added 1 - Planned and removed 0 - Backlog labels Oct 6, 2014

@andyberry88 andyberry88 added size 4 and removed size 3 labels Oct 13, 2014

@andyberry88 andyberry88 added 2 - Dev and removed 1 - Planned labels Nov 25, 2014

@andyberry88 andyberry88 self-assigned this Nov 25, 2014

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Nov 27, 2014

Member

Since there isn't a huge amount of acceptance criteria around this, heres my current plan:

  • Allowing the final <path> argument to be optional is difficult since its ambiguous whether a user meant to leave out the argument or forgot it - so it looks like this might have to be a flagged option to prevent the ambiguity.
  • Commands that are 'context aware' must be run within the context they expect. For example create-blade would expect to be run within a BladeSet, so if it was run within the apps directory it would throw an exception. This means the commands that previously took extra arguments to define what was being talked about create-blade/create-bladeset etc no longer need these arguments.
  • Commands that choose not to be context aware work exactly as they did before.
  • The context path is set by the current working directory and can be optionally set by using --context <some-path>. This means commands can still be run from anywhere if this flag is used.
  • Commands can be optionally context aware. The AbstractContentPlugin class will define the relevant methods so any classes extending it will not need to change when the CommandPlugin API changes - these classes continue to implement doCommand(String... args).
  • Any commands that are now context aware should extend ContextAwareAbstractContentPlugin and implement the method doCommand(File contextDir, Node contextNode, String... args).
  • The model can do a lot of the common things like listing expected contexts in the help command output, ensuring the correct context is used when running a command etc.

Most of the groundwork for this has been done, its just the commands that now all need converting to be context aware, but I wanted to get some feedback before ploughing ahead.

@dchambers @james-shaw-turner @leggetter @thecapdan @ioanalianabalas any thoughts on this?

Member

andyberry88 commented Nov 27, 2014

Since there isn't a huge amount of acceptance criteria around this, heres my current plan:

  • Allowing the final <path> argument to be optional is difficult since its ambiguous whether a user meant to leave out the argument or forgot it - so it looks like this might have to be a flagged option to prevent the ambiguity.
  • Commands that are 'context aware' must be run within the context they expect. For example create-blade would expect to be run within a BladeSet, so if it was run within the apps directory it would throw an exception. This means the commands that previously took extra arguments to define what was being talked about create-blade/create-bladeset etc no longer need these arguments.
  • Commands that choose not to be context aware work exactly as they did before.
  • The context path is set by the current working directory and can be optionally set by using --context <some-path>. This means commands can still be run from anywhere if this flag is used.
  • Commands can be optionally context aware. The AbstractContentPlugin class will define the relevant methods so any classes extending it will not need to change when the CommandPlugin API changes - these classes continue to implement doCommand(String... args).
  • Any commands that are now context aware should extend ContextAwareAbstractContentPlugin and implement the method doCommand(File contextDir, Node contextNode, String... args).
  • The model can do a lot of the common things like listing expected contexts in the help command output, ensuring the correct context is used when running a command etc.

Most of the groundwork for this has been done, its just the commands that now all need converting to be context aware, but I wanted to get some feedback before ploughing ahead.

@dchambers @james-shaw-turner @leggetter @thecapdan @ioanalianabalas any thoughts on this?

@andyberry88 andyberry88 added the feature label Nov 27, 2014

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Nov 27, 2014

Contributor

I'd prefer the final parameter to be the optional context e.g.

cd apps/my-app/blades

# creates blade called fish in current directory
../../../sdk/ create-blade fish

# creates blade in other-app/blades directory
../../../sdk/ create-blade fish ../../../apps/other-app/blades

Seeing this highlights that the necessity to do ../../../sdk/ really isn't nice. So, I think we should make sure having brjs on the PATH makes this a nicer experience.

Longer term, we should consider doing what I think Grunt and Gulp do - have a global runner which checks to see if there is a local version of the CLI to actually use. More info available on this if required.

Contributor

leggetter commented Nov 27, 2014

I'd prefer the final parameter to be the optional context e.g.

cd apps/my-app/blades

# creates blade called fish in current directory
../../../sdk/ create-blade fish

# creates blade in other-app/blades directory
../../../sdk/ create-blade fish ../../../apps/other-app/blades

Seeing this highlights that the necessity to do ../../../sdk/ really isn't nice. So, I think we should make sure having brjs on the PATH makes this a nicer experience.

Longer term, we should consider doing what I think Grunt and Gulp do - have a global runner which checks to see if there is a local version of the CLI to actually use. More info available on this if required.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Nov 27, 2014

Member

I'd prefer the final parameter to be the optional context e.g.

Thats what I've suggested above. It can't be a straightforward argument though since the command arguments are then ambiguous. But we can make it an optional flagged option, e.g.

cd apps/my-app/blades

# creates blade called fish in current directory
../../../sdk/ create-blade fish

# creates blade in other-app/blades directory
../../../sdk/ create-blade fish --context ../../../apps/other-app/blades

Seeing this highlights that the necessity to do ../../../sdk/ really isn't nice. So, I think we should make sure having brjs on the PATH makes this a nicer experience.

Yep, agreed. For the time being, assuming only one BRJS instance is on the path, this should work in a reasonably nice way. When we look at the full global install and apps separate from the BRJS install we'll need to refine how this works.

Member

andyberry88 commented Nov 27, 2014

I'd prefer the final parameter to be the optional context e.g.

Thats what I've suggested above. It can't be a straightforward argument though since the command arguments are then ambiguous. But we can make it an optional flagged option, e.g.

cd apps/my-app/blades

# creates blade called fish in current directory
../../../sdk/ create-blade fish

# creates blade in other-app/blades directory
../../../sdk/ create-blade fish --context ../../../apps/other-app/blades

Seeing this highlights that the necessity to do ../../../sdk/ really isn't nice. So, I think we should make sure having brjs on the PATH makes this a nicer experience.

Yep, agreed. For the time being, assuming only one BRJS instance is on the path, this should work in a reasonably nice way. When we look at the full global install and apps separate from the BRJS install we'll need to refine how this works.

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Nov 27, 2014

Contributor

I really don't like the --context. I'm sure lots of other CLIs run in the current directory (context) unless a final path parameter is passed. Is there no way of BRJS achieving this?

Contributor

leggetter commented Nov 27, 2014

I really don't like the --context. I'm sure lots of other CLIs run in the current directory (context) unless a final path parameter is passed. Is there no way of BRJS achieving this?

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Nov 27, 2014

Member

We can achieve it, anything is possible 😄. The thing that makes it more difficult for us is supporting pluggable commands but trying to also augment command arguments from the outside so any model/BRJS concerns don't need to be handled by plugins.

Command plugins configure the options they support before a command is even run, and before the context is known. So for example create-blade defines its arguments as create-blade <app-name> <bladeset-name> <blade-name>, but if the command is run from an App it becomes create-blade <bladeset-name> <blade-name>, and from a BladeSet it becomes create-blade <blade-name>.

The possible ways to support an optional fifth argument might be:

  • Change commands so if they are context aware commands they must define a different set of arguments for every context they want to support - this might be in the form of a map where Context -> CommandArgs. Depending on how many contexts a commands supports this might become very painful when writing command plugins.
    • As I think about it this may be a nice option as far as the code in the model goes. My main concern is how messy command plugins might become if they have to now define multiple argument types.
  • Attempt the guess whether the last argument is the context directory. This might be difficult to do since we have several commands that have a file path as the last argument (build-app app-name path-to-built-app-dir) so its not as straightforward as checking if the last argument is a directory.

Theres always the option of not allowing the the context parameter and requiring commands to be run from the correct directory.

Member

andyberry88 commented Nov 27, 2014

We can achieve it, anything is possible 😄. The thing that makes it more difficult for us is supporting pluggable commands but trying to also augment command arguments from the outside so any model/BRJS concerns don't need to be handled by plugins.

Command plugins configure the options they support before a command is even run, and before the context is known. So for example create-blade defines its arguments as create-blade <app-name> <bladeset-name> <blade-name>, but if the command is run from an App it becomes create-blade <bladeset-name> <blade-name>, and from a BladeSet it becomes create-blade <blade-name>.

The possible ways to support an optional fifth argument might be:

  • Change commands so if they are context aware commands they must define a different set of arguments for every context they want to support - this might be in the form of a map where Context -> CommandArgs. Depending on how many contexts a commands supports this might become very painful when writing command plugins.
    • As I think about it this may be a nice option as far as the code in the model goes. My main concern is how messy command plugins might become if they have to now define multiple argument types.
  • Attempt the guess whether the last argument is the context directory. This might be difficult to do since we have several commands that have a file path as the last argument (build-app app-name path-to-built-app-dir) so its not as straightforward as checking if the last argument is a directory.

Theres always the option of not allowing the the context parameter and requiring commands to be run from the correct directory.

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Nov 27, 2014

Contributor

Can't commands just define their last argument as greedy?
http://www.martiansoftware.com/jsap/doc/ch08.html

Contributor

leggetter commented Nov 27, 2014

Can't commands just define their last argument as greedy?
http://www.martiansoftware.com/jsap/doc/ch08.html

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Nov 27, 2014

Member

I'm not sure how that would help. We need to know whether the last arg is the context or not so so we can provide that as the context to the plugin.

Member

andyberry88 commented Nov 27, 2014

I'm not sure how that would help. We need to know whether the last arg is the context or not so so we can provide that as the context to the plugin.

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Nov 27, 2014

Contributor

Couldn't we leave it up to the plugins to decide whether to use the context
or not?

If they get their own final greedy argument they use that. If not, they use
the context.

Contributor

leggetter commented Nov 27, 2014

Couldn't we leave it up to the plugins to decide whether to use the context
or not?

If they get their own final greedy argument they use that. If not, they use
the context.

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Nov 27, 2014

Contributor

I haven't got time to read any of these, but it seems like we should halt development till after we've had a get together with the following people: @andyberry88, @leggetter, @thecapdan, @james-shaw-turner and myself.

Contributor

dchambers commented Nov 27, 2014

I haven't got time to read any of these, but it seems like we should halt development till after we've had a get together with the following people: @andyberry88, @leggetter, @thecapdan, @james-shaw-turner and myself.

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Nov 27, 2014

Contributor

I'll be available between 3:45 and 4:45 BTW.

Contributor

dchambers commented Nov 27, 2014

I'll be available between 3:45 and 4:45 BTW.

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Nov 27, 2014

Contributor

As things currently stand, to create an app, then create a blade within that app, then run the blade tests, I need to run the following commands:

./brjs create-app my-app
./brjs create-blade my-app default b1
./brjs test ../apps/my-app/blades/b1/

If brjs could be put on the path I could instead run these commands:

brjs create-app my-app
brjs create-blade my-app default b1
brjs test ../apps/my-app/blades/b1/

If we implemented the context-aware commands proposal I could instead run these commands:

brjs create-app my-app
cd my-app
brjs create-blade b1
brjs test blades/b1
Contributor

dchambers commented Nov 27, 2014

As things currently stand, to create an app, then create a blade within that app, then run the blade tests, I need to run the following commands:

./brjs create-app my-app
./brjs create-blade my-app default b1
./brjs test ../apps/my-app/blades/b1/

If brjs could be put on the path I could instead run these commands:

brjs create-app my-app
brjs create-blade my-app default b1
brjs test ../apps/my-app/blades/b1/

If we implemented the context-aware commands proposal I could instead run these commands:

brjs create-app my-app
cd my-app
brjs create-blade b1
brjs test blades/b1
@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Dec 2, 2014

Member

There is a slightly more in depth write up here.

I suggest we use this issue for and comments/feedback on the proposal since wiki pages don't support comments.

Member

andyberry88 commented Dec 2, 2014

There is a slightly more in depth write up here.

I suggest we use this issue for and comments/feedback on the proposal since wiki pages don't support comments.

@janhancic

This comment has been minimized.

Show comment
Hide comment
@janhancic

janhancic Dec 2, 2014

Contributor

I don't really care about the create blade etc commands, as I rarely use them. But I would love to be able to do this for running the tests:

cd apps/my-app
brjs test libs/my-lib UTs

But I shouldn't be forced to cd into the correct folder. I should be able to provide the path that overrides the current PWD. Basically, just make it work as any *nix program.

Contributor

janhancic commented Dec 2, 2014

I don't really care about the create blade etc commands, as I rarely use them. But I would love to be able to do this for running the tests:

cd apps/my-app
brjs test libs/my-lib UTs

But I shouldn't be forced to cd into the correct folder. I should be able to provide the path that overrides the current PWD. Basically, just make it work as any *nix program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment