qooxdoo-archive / qooxdoo-cli

(deprecated, moved into qooxdoo-compiler) qx commandline
MIT License
4 stars 5 forks source link

Add class template system #82

Closed cboulanger closed 6 years ago

cboulanger commented 6 years ago

Because of the complex nature of qooxdoo class declaration, creating them takes some effort, in particular if they should follow the qooxdoo framework style. Personally, this is one reason I often add stuff to existing classes instead of putting it into new ones, which would be better in terms of readability and application architecture. This issue proposes to add a command qx add ${template name} ${class name}, which will work the following way:

  1. It would have to be executed in the project root dir and expect Manifest.json to exist there (this could be configurable). In any case, it would expect the source folder to be a subdirectory of the current dir.
  2. It would look up the template file like so: If ${template name} is a simple string such as "bar", the path would be resolved as templates/bar.tmpl.js appended to the path to the qxcli folder. If it is of the form "foo/bar", this the path would be resolved as templates/bar.tmpl.js appended to the path of the contrib library "foo". This way, the template system can be extended by contribs.
  3. It would flatten the properties of Manifest.json and allow to use them in the templates, plus some other properties. It would write out the file with the template vars replaced according to the namespace path of ${class name}.

An implementation PR follows, but I'd like to get your opinion as early as possible.

cboulanger commented 6 years ago

Had to edit the first comment because I messed up the markdown...

oetiker commented 6 years ago

hmmm I don't quite get yet what you are trying to optimize ... but I am sure the PR will show ... the only thing I would love to see, the option to create my own class template inside the project so that the license boilerplate gets added automatically

cboulanger commented 6 years ago

What I'd like to be able to do is the following

qx add class myproject.ui.LoginWindow --extend=qx.ui.window.Window

this would create a class file path/to/myproject/ui/LoginWindow.js with all bells and whistles (constructor, properties, static, members, dispose) ready for coding.

qx add dialog/loginwindow myproject.ui.LoginWindow

This would take a (not yet existing) template from the dialog contrib which would implement a fully functional Login Window out-of-the-box ready for adapting it.

Good idea to add support for project templates! What about this lookup algorithm:

cboulanger commented 6 years ago

Slight change in PR #83: template files are not in the templates dir, but in the templates/class dir.

oetiker commented 6 years ago

ah ... ok ... re your proposed syntax above ... might be a tad problematic since you are mixing key words (class) and path names (dialog/...) in the same position. also 'add' seems a bit generic for what you have in mind ...

how about

qx createclass qx.ui.window.Window myproject.ui.Login

cboulanger commented 6 years ago

you're right that the contrib template solution is not ideal yet. But I am not convinced about the syntax you propose. One can immediately understand what qx add class x --extend=y means, whereas it is not clear what your command would do.

cboulanger commented 6 years ago

May for contrib templates, the syntax could be qx add loginwindow --from-contrib=dialog myproject.LoginWindow

cboulanger commented 6 years ago

One addition could be a --copy or --customize flag that would copy the template from its origin to the "templates/class" directory of the current project, where it could be customized for the project

derrell commented 6 years ago

What would be in the template? When I'm creating a new class, I just copy the first 50 lines or so of some other class from my app, which includes the copyright notice and such, qx.Class.define line. Everything else is dependent on the needs of the class. In fact, using a template from a contrib would have the wrong copyright notice, since this new class should have my app's copyright, not the contrib's copyright... Maybe I'm just not understand everything that this new command would do?

cboulanger commented 6 years ago

@derrell No, the template would not be a class file, it would be a "template", i.e. it would contain template variables instead of the copyright notice :-)

The whole idea is

a) convenience, especially for beginners, who do not have existing classes to copy from. You qx create an application, then qx add a couple of classes, interfaces, mixins etc., and just fill in logic. Remember that qooxdoo has a steep learning curve, so being able to generate code quickly is a win.

b) the ability to quickly generate functioning code from templates for all kinds of purposes. This will of course require some additional logic, but this could be a basis.

johnspackman commented 6 years ago

Could we have a top-level file in the project which is the usual copyright header, so that there can be a standard set of templates which can work for most users; the skeleton's created by qx could create a sample version of this file.

What I'd love to see (but may be outside the scope of this) is to be able to edit that file and have those changes copied into existing sources. This would depend on formalising the structure slightly, eg so that the authors are not replaced.

cboulanger commented 6 years ago

as to 1), do you mean that the copyright header in its entirety should be a template variable, which is then added on the top of the actual source files, instead of putting template variables into headers in each individual source file? Sounds reasonable!

as to 2) yes definitely beyond this PR but could certainly be done.

level420 commented 6 years ago

I'd vote to have source files which do not contain any macro/template variable regarding the copyright, but the already resolved copyright text. Imagine you're reading one of those macro-ized source files on github via the browser and find some macro ${copyright} in the header with no simple way to find out what will be placed in there.

cboulanger commented 6 years ago

@level420 I just added this feature. Here's the current description:

qx add <template> <classname> [options]

Options:
  --extend, -e  the class to extend
  --import      import the template to the `templates/class` folder of the
                current project, where it can be customized
  --force, -f   overwrite an existing file

/**
 * Add a new class file to the current project, based on a template.
 *
 * Syntax: `qx add template_name class_name [--extend=extended_class] [--import] [--from-contrib=contrib_name]`
 * The path to the template file will be calculated as follows:
 * 1. transform template_name to ${template_name}.tmpl.js
 * 2. check if this file exists in the "templates/class" folder 
 *    a. of a contrib, if the --from-contrib paramater wass passed
 *    b. otherwise, of the current project
 *    c. finally, of the CLI library
 *
 * If the --import flag is set, copy the file to the templates/class folder of the current project.
 * 
 * If you place a file named `header.js` in the root of your project, this header will be used verbatim
 * as the ${header} template variable instead of the generic header template, which is populated with 
 * information from Manifest.json
 * 
 * (Contrib support is not yet implemented)
 * 
 */
oetiker commented 6 years ago

calling the thing add seems a bit pretentious ... you could be adding many things ... why do new classes get that most generic of keywords?

johnspackman commented 6 years ago

@level420 I agree, the source files should have expanded text in them; what I was thinking was that our headers are usually very uniform and it should be possible to figure out the template values that were used to create the header - this would be either by comparing the original template to the header, or just by requiring that the header follows a fixed format (eg: "Authors" on one line, followed by a list of authors, as in our headers). Once you know the template values that were used to create the file, you can rewrite the header using the new template and those values.

cboulanger commented 6 years ago

@oetiker In the end, I don't really care what keyword we use, butI don't fully understand what bothers you about "add" so much. In my perspective, it seems to be quite natural to first "create" an application and then "add" elements to it. What more sophisticated usage of "add" do you have in mind that you want to reserve the keyword for? As almost everything in qooxdoo is a class, most things that will be "added" are classes, but it doesn't need to be limited to this. It could be expanded to add a new theme, resources, etc. (although I haven't thought through how useful that would be in practice - just in principle). We are not limited to template files with template vars in them -template_name could be a javascript file which does all sorts of complex things and adds the result.

What do the others think? Is "add" (as opposed to "create") the wrong choice of keyword here?

oetiker commented 6 years ago

@cboulanger I was more thinking in the sense that qx will be the 'everything' you do with a qooxdoo app ... creating, compiling, linting, reformatting, but also handling contribt, creating contribs, publishing contribs ... and we might spend some thinking about making the cli API consistent while we still can ... once many people use it we are stuck with it ...

cboulanger commented 6 years ago

@oetiker I totally agree with you. We need to choose our API terms and syntax carefully. That is why I would like to keep the syntax for qx add as generic and declarative as possible, so that it can be extended to "add" almost everything in the future. But it is good to critically examine it.

level420 commented 6 years ago

what about having the generic add followed by a specific keyword like

qx add classfile <template> <classname> [options]

This way we are open for other 'add'-able things like qx add webfont or whatever may come. In general I'd prefer adding existing things and createing new things

cboulanger commented 6 years ago

Sounds good to me. Maybe we could use a default template "class" (which would either be the generic template or a user-defined one) and use the following syntax:

qx add classfile myApp.ui.LoginWindow

which would use theclass.tmpl.js template, and

qx add classfile --type=interface myApp.plugin.IPlugin

for using interface.tmpl.js. On the other hand, being able to use the template type without prefixing is more easy to write.

cboulanger commented 6 years ago

No, a default type "class" doesn't make sense if we have other things like webfonts. Retracting last comment...

cboulanger commented 6 years ago

Or the default type name is "default". Then it could be left out in each type of "Addable", which would then use the "default" template.

level420 commented 6 years ago

or

qx create interface <name> [derivfromname]
qx create class <name> [derivefromname]
qx create mixin <name> [derivefromname]

with concrete

qx create class myApp.ui.LoginWindow genericApp.ui.LoginWindow

which generates a class file deriving from genericApp.ui.LoginWindow.

oetiker commented 6 years ago

perfect ... that is the kind of discussion I was looking for :)

when designing cli interfaces, I try to strongly limit the number of positional (non-keyword) arguments ... all the optional things get --option=value names ... going for optional positional parameters (especially if they are not keywords) can severely limit the future extensibility of the API.

Also when mixing optional keywords and values we expose ourselves to the danger of miss interpretation.

level420 commented 6 years ago

yes maybe better to have:

qx create class myApp.ui.LoginWindow --base=genericApp.ui.LoginWindow
oetiker commented 6 years ago

@level420 and have some application config file where the defaults can be set in a sensible manner

level420 commented 6 years ago

create.class.base = qx.core.Object or similar

cboulanger commented 6 years ago

To be honest, I wouldn't want to mix create and add because they have totally different information needs. When you "create" an application, you need all kind of data for Manifest.json and compile.json, which is provided by CLI options or inquired interactively. When you "add" an application component, you already have all that information available and can use it to selectively extend the existing app with some new element. So lumping these two different use cases into one seems to be bad both in design and confusing for the user, IMHO. -- Update -- Also, qx create can be executed in any directory, creating (by default) the application in a subfolder, whereas qx add needs to be executed in the application root. Another reason not to merge them.

cboulanger commented 6 years ago

What abou this: we make qx add a meta- command such as qx contrib, and then have separate subcommands, class being one of them?

cboulanger commented 6 years ago

Ok, with https://github.com/qooxdoo/qooxdoo-cli/pull/83/commits/7aafc54749ae342e7660e99e995f2c6a5aaf0ceb, we have this:

/**
 * Add a new class file to the current project, based on a template.
 *
 * Syntax: `qx add class <classname> [--type=template_name] [--extend=extended_class] [--import] [--from-contrib=contrib_name]`
 * If omitted, `--type` defaults to "default". The path to the template file will be calculated as follows:
 * 1. transform template_name to ${template_name}.tmpl.js
 * 2. check if this file exists in the "templates/class" folder 
 *    a. of a contrib, if the --from-contrib paramater wass passed (not implemented)
 *    b. otherwise, of the current project
 *    c. finally, of the CLI library
 *
 * If the --import flag is set, copy the template to the templates/class folder of the current project,
 * so it can be customized and used instead of the one shipped with the CLI.
 * 
 * If you place a file named `header.js` in the root of your project, this header will be used verbatim
 * as the ${header} template variable instead of the generic header template, which is populated with 
 * information from Manifest.json
 * 
 * (Contrib support is not yet implemented)
 * 
 */

class is now a subcommand of add. It doesn't support @level420's idea of deriving classes from a generic app, but I haven't fully understood how that should work ... :-)

level420 commented 6 years ago

@cboulanger I didn't meant that it should derive or import anything from other classes (maybe wrong worded). I meant the same what I think you have with --extend=extended_class.

cboulanger commented 6 years ago

@level420 qx add webfont sounds like a great idea. That would make a somewhat complicated setup really easy (and make for a great tutorial) :-)

hkollmann commented 6 years ago

base done with #83