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

BladeSets should be optional #2

Closed
leggetter opened this Issue Sep 24, 2013 · 13 comments

Comments

Projects
None yet
6 participants
@leggetter
Contributor

leggetter commented Sep 24, 2013

BladeSets are a concept that have their uses, but aren't required to start of with.

Internally this may mean that there is a default BladeSet that doesn't reside within a bladeset- directory. But for the developer PoV there isn't a BladeSet.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 6, 2014

Member

#19 contains the discussion about how we'll handle the 'default' bladeset.

As part of this the default for the create-blade command should be to create a blade in the 'default' bladeset. We can't make the middle argument of the current command optional (e.g. brjs create-blade app-name [bladeset-name] blade-name) since its ambiguous whether the third argument is missing or it is intentionally not there.

Given new users (and probably the majority of OSS users) of BRJS won't use bladesets it would seem sensible to make the default to create the blade without the bladeset and provide a flag to specify whether a bladeset should be used. The command would then look like brjs create-blade app-name blade-name [-bset myBladeset].

@dchambers @leggetter @bit-shifter @stevesouth any thoughts on this?

Member

andyberry88 commented Aug 6, 2014

#19 contains the discussion about how we'll handle the 'default' bladeset.

As part of this the default for the create-blade command should be to create a blade in the 'default' bladeset. We can't make the middle argument of the current command optional (e.g. brjs create-blade app-name [bladeset-name] blade-name) since its ambiguous whether the third argument is missing or it is intentionally not there.

Given new users (and probably the majority of OSS users) of BRJS won't use bladesets it would seem sensible to make the default to create the blade without the bladeset and provide a flag to specify whether a bladeset should be used. The command would then look like brjs create-blade app-name blade-name [-bset myBladeset].

@dchambers @leggetter @bit-shifter @stevesouth any thoughts on this?

@bit-shifter

This comment has been minimized.

Show comment
Hide comment
@bit-shifter

bit-shifter Aug 6, 2014

Contributor

If you are intending to change the command; moving the bladeset flag/argument to the end of the command, why not go to the logical conclusion and allow the bladeset argument to be optional*?

  • documented in the CLI of course
Contributor

bit-shifter commented Aug 6, 2014

If you are intending to change the command; moving the bladeset flag/argument to the end of the command, why not go to the logical conclusion and allow the bladeset argument to be optional*?

  • documented in the CLI of course
@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 6, 2014

Member

why not go to the logical conclusion and allow the bladeset argument to be optional

That is what I'm suggesting. So the valid uses would be brjs create-blade app-name blade-name and brjs create-blade app-name blade-name -bset bladeset-name.

Member

andyberry88 commented Aug 6, 2014

why not go to the logical conclusion and allow the bladeset argument to be optional

That is what I'm suggesting. So the valid uses would be brjs create-blade app-name blade-name and brjs create-blade app-name blade-name -bset bladeset-name.

@bit-shifter

This comment has been minimized.

Show comment
Hide comment
@bit-shifter

bit-shifter Aug 6, 2014

Contributor

Yes, I'm suggesting that the last two arguments are simply switched, I don't see the need for the -bset flag.

If a bladeset name isn't provided (arg[2]), then create the blade in the default aspect.

Contributor

bit-shifter commented Aug 6, 2014

Yes, I'm suggesting that the last two arguments are simply switched, I don't see the need for the -bset flag.

If a bladeset name isn't provided (arg[2]), then create the blade in the default aspect.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 6, 2014

Member

Since blades live inside bladesets (the structure is app -> bladeset -> blade) IMO it would be confusing to use the arguments in a different order. The added flag seperates it slightly so its more obvious that's the bladeset.

For example brjs create-blade foo bar baz - which is the bladeset? Whereas brjs create-blade foo bar -bset baz is slightly more explicit.

Member

andyberry88 commented Aug 6, 2014

Since blades live inside bladesets (the structure is app -> bladeset -> blade) IMO it would be confusing to use the arguments in a different order. The added flag seperates it slightly so its more obvious that's the bladeset.

For example brjs create-blade foo bar baz - which is the bladeset? Whereas brjs create-blade foo bar -bset baz is slightly more explicit.

@bit-shifter

This comment has been minimized.

Show comment
Hide comment
@bit-shifter

bit-shifter Aug 6, 2014

Contributor

Hmm, it would be useful to hear others' feedback on this issue.

Whilst I agree that the ordering is unnatural given the resultant directory structure, I don't see that adding the -bset prelude adds any real benefit, and could be considered an annoying necessity if using bladesets in typical development.

As you say, the majority of users would not be using bladesets, so it would be clear to those users (at some point where bladesets become applicable) that the bladeset (optional) name would be the last argument.

This detail would be covered in the command's help in the CLI, so I don't see a reason for concern.

Contributor

bit-shifter commented Aug 6, 2014

Hmm, it would be useful to hear others' feedback on this issue.

Whilst I agree that the ordering is unnatural given the resultant directory structure, I don't see that adding the -bset prelude adds any real benefit, and could be considered an annoying necessity if using bladesets in typical development.

As you say, the majority of users would not be using bladesets, so it would be clear to those users (at some point where bladesets become applicable) that the bladeset (optional) name would be the last argument.

This detail would be covered in the command's help in the CLI, so I don't see a reason for concern.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 8, 2014

Member

After talking with @leggetter and @dchambers we've decided to keep the command arguments in the same order and address that as part of #885. See #885 for ongoing discussion of 'context' aware commands.

Member

andyberry88 commented Aug 8, 2014

After talking with @leggetter and @dchambers we've decided to keep the command arguments in the same order and address that as part of #885. See #885 for ongoing discussion of 'context' aware commands.

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Aug 22, 2014

Member

Okay so I'm seeing a fair number of issues with this, here's what the current develop branch behaviour is:

  1. Create Blade command
  • Should create it inside the 'default' bladeset
  • brjs create-blade <appName> default <bladeName> results in a structure which is a cross between 3/4 from #19 as we have optional aspects now:
+- myapp
|   \- blades
|       \- blade1
|           \- src
|               \- myapp
|                   \- default
|                       -- Blade1.js
|   \- libs
|   \- resources (resources for Default Aspect)
|   \- src (JS source for Default Aspect)
|       \- myapp
|           -- Myapp.js
|   \- themes (themes for Default Aspect)
|   -- app.conf
|   -- index.html (Default Aspect seed file)
|   \- libs
|   -- app.conf

Is this the structure we were expecting on disk? (I'm aware there's a separate issue for package structure simplification)

  1. Require paths for blades in default bladeset
  2. @andyberry88: "I assume the require path for a blade that doesn't have a bladeset becomes"
  3. @leggetter: "Yes"

Given the above, here are the results I get when I try and require the Blade1.js file from my application index.html:

*A: Requiring without bladeset or default in the path *

require ( 'myapp/blade1/Blade1' );

java.lang.RuntimeException: org.bladerunnerjs.model.exception.InvalidRequirePathException`:

The source module at 'blades/hawx/src/woop/default/hawx/Enigma.js' is in an invalid location. It's require path starts with the apps require prefix ('woop') which suggests it's require path is intended to be 'woop/hawx'. The require path defined by the source modules location is 'woop/default/hawx'. Either it's package structure should be 'woop/hawx/*' or remove the folders 'woop/default/hawx' to allow the require prefix to be calculated automatically.

Okay so it sounds like it's giving me some good suggestions.. so I try with default in the path first:

**B: Requiring with default in the path **

require ( 'myapp/default/blade1/Blade1' );

java.lang.RuntimeException: org.bladerunnerjs.model.exception.InvalidRequirePathException: 

The source module at 'blades/hawx/src/woop/default/hawx/Enigma.js' is in an invalid location. It's require path starts with the apps require prefix ('woop') which suggests it's require path is intended to be 'woop/hawx'. The require path defined by the source modules location is 'woop/default/hawx'. Either it's package structure should be 'woop/hawx/*' or remove the folders 'woop/default/hawx' to allow the require prefix to be calculated automatically.

Still no luck, I then change the folder structure of the blade so that it's at the root of the src folder:

+- myapp
|   \- blades
|       \- blade1
|           \- src
|               -- Blade1.js

require ( 'myapp/default/blade1/Blade1' ); - results in JS error
require ( 'myapp/blade1/Blade1' ); - works OK

In Summary

  • blade template structure is wrong for this issue to work for users
  • exception error messages could be improved (all on one huge single line)

Still to do

  • how this affects namespace style? need to test
  • more spec tests
Member

sospirited commented Aug 22, 2014

Okay so I'm seeing a fair number of issues with this, here's what the current develop branch behaviour is:

  1. Create Blade command
  • Should create it inside the 'default' bladeset
  • brjs create-blade <appName> default <bladeName> results in a structure which is a cross between 3/4 from #19 as we have optional aspects now:
+- myapp
|   \- blades
|       \- blade1
|           \- src
|               \- myapp
|                   \- default
|                       -- Blade1.js
|   \- libs
|   \- resources (resources for Default Aspect)
|   \- src (JS source for Default Aspect)
|       \- myapp
|           -- Myapp.js
|   \- themes (themes for Default Aspect)
|   -- app.conf
|   -- index.html (Default Aspect seed file)
|   \- libs
|   -- app.conf

Is this the structure we were expecting on disk? (I'm aware there's a separate issue for package structure simplification)

  1. Require paths for blades in default bladeset
  2. @andyberry88: "I assume the require path for a blade that doesn't have a bladeset becomes"
  3. @leggetter: "Yes"

Given the above, here are the results I get when I try and require the Blade1.js file from my application index.html:

*A: Requiring without bladeset or default in the path *

require ( 'myapp/blade1/Blade1' );

java.lang.RuntimeException: org.bladerunnerjs.model.exception.InvalidRequirePathException`:

The source module at 'blades/hawx/src/woop/default/hawx/Enigma.js' is in an invalid location. It's require path starts with the apps require prefix ('woop') which suggests it's require path is intended to be 'woop/hawx'. The require path defined by the source modules location is 'woop/default/hawx'. Either it's package structure should be 'woop/hawx/*' or remove the folders 'woop/default/hawx' to allow the require prefix to be calculated automatically.

Okay so it sounds like it's giving me some good suggestions.. so I try with default in the path first:

**B: Requiring with default in the path **

require ( 'myapp/default/blade1/Blade1' );

java.lang.RuntimeException: org.bladerunnerjs.model.exception.InvalidRequirePathException: 

The source module at 'blades/hawx/src/woop/default/hawx/Enigma.js' is in an invalid location. It's require path starts with the apps require prefix ('woop') which suggests it's require path is intended to be 'woop/hawx'. The require path defined by the source modules location is 'woop/default/hawx'. Either it's package structure should be 'woop/hawx/*' or remove the folders 'woop/default/hawx' to allow the require prefix to be calculated automatically.

Still no luck, I then change the folder structure of the blade so that it's at the root of the src folder:

+- myapp
|   \- blades
|       \- blade1
|           \- src
|               -- Blade1.js

require ( 'myapp/default/blade1/Blade1' ); - results in JS error
require ( 'myapp/blade1/Blade1' ); - works OK

In Summary

  • blade template structure is wrong for this issue to work for users
  • exception error messages could be improved (all on one huge single line)

Still to do

  • how this affects namespace style? need to test
  • more spec tests
@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 22, 2014

Member

blade template structure is wrong for this issue to work for users

The templates have been updated so they don't include package directories. That was causing the problem where the default directory was being included where it shouldn't be.

exception error messages could be improved (all on one huge single line)

This is not specific to this issue. Any exception that is thrown and displayed on a webpage isn't formatted particularly well. Probably needs to be fixed although on the scale of things I'm not sure how high this is on the priority list.

how this affects namespace style? need to test
more spec tests

It doesn't affect dependencies or requires. It only affects the require path for Source Modules. There are several new tests in AspectBundlingOfMixedSources that directly test this and several others for the dependency analysis commands to test the correct path is output for these source files. There are also some existing tests that now use the new package structure since they're using the model to construct the app under test.

Member

andyberry88 commented Aug 22, 2014

blade template structure is wrong for this issue to work for users

The templates have been updated so they don't include package directories. That was causing the problem where the default directory was being included where it shouldn't be.

exception error messages could be improved (all on one huge single line)

This is not specific to this issue. Any exception that is thrown and displayed on a webpage isn't formatted particularly well. Probably needs to be fixed although on the scale of things I'm not sure how high this is on the priority list.

how this affects namespace style? need to test
more spec tests

It doesn't affect dependencies or requires. It only affects the require path for Source Modules. There are several new tests in AspectBundlingOfMixedSources that directly test this and several others for the dependency analysis commands to test the correct path is output for these source files. There are also some existing tests that now use the new package structure since they're using the model to construct the app under test.

@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 27, 2014

Member

@leggetter has raised another issue (#917) which is a duplicate of this but contains some info about an exception that is now seen (#917 (comment)).

Member

andyberry88 commented Aug 27, 2014

@leggetter has raised another issue (#917) which is a duplicate of this but contains some info about an exception that is now seen (#917 (comment)).

@thanhc

This comment has been minimized.

Show comment
Hide comment
@thanhc

thanhc Aug 28, 2014

Contributor

Tested and verified.
Just to highlight a few points:

  • when creating an app the blades/ is not created by default (this is only created when you create a blade with default bladeset)
  • require ( 'myapp/blade1/Blade1' ); - works OK but using default doesn't work as discussed - this is the same behavior for the classpaths.
  • if you create a default-bladeset this does not act as the default and therefore everything has to be namespaced correctly the the default bladeset.
Contributor

thanhc commented Aug 28, 2014

Tested and verified.
Just to highlight a few points:

  • when creating an app the blades/ is not created by default (this is only created when you create a blade with default bladeset)
  • require ( 'myapp/blade1/Blade1' ); - works OK but using default doesn't work as discussed - this is the same behavior for the classpaths.
  • if you create a default-bladeset this does not act as the default and therefore everything has to be namespaced correctly the the default bladeset.
@andyberry88

This comment has been minimized.

Show comment
Hide comment
@andyberry88

andyberry88 Aug 28, 2014

Member

when creating an app the blades/ is not created by default (this is only created when you create a blade with default bladeset)

This will change once #933 has been merged.

@thanhc I assume you were just pointing out the other two rather than raising them as things to be fixed?

Member

andyberry88 commented Aug 28, 2014

when creating an app the blades/ is not created by default (this is only created when you create a blade with default bladeset)

This will change once #933 has been merged.

@thanhc I assume you were just pointing out the other two rather than raising them as things to be fixed?

@thanhc thanhc added 4 - Test and removed 3 - ReadyForTest labels Aug 29, 2014

@thanhc

This comment has been minimized.

Show comment
Hide comment
@thanhc

thanhc Aug 29, 2014

Contributor

Yes, I was just making it clear the expected behavior.

Contributor

thanhc commented Aug 29, 2014

Yes, I was just making it clear the expected behavior.

@thanhc thanhc added 5 - Done and removed 4 - Test labels Aug 29, 2014

@andyberry88 andyberry88 removed the 5 - Done label Sep 4, 2014

@andyberry88 andyberry88 assigned dchambers and unassigned sospirited Dec 3, 2014

thecapdan added a commit that referenced this issue Dec 3, 2014

thanhc added a commit that referenced this issue Mar 25, 2015

thanhc added a commit that referenced this issue Mar 26, 2015

This issue was closed.

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