search

zen0wu / topcoder-greed

greedy editor for topcoder arena
Apache License 2.0
229 stars 45 forks source link

DSL Configuration syntax - "bultin" is awkward. #76

Closed wookayin closed 10 years ago

wookayin commented 10 years ago

For example, in greed.conf, we have

greed.language.cpp.templateDef {
    source.templateFile = "builtin testcase/testcases.tmpl"
}

The builtin prefix is introduced in fd295044, and it seems not documented anywhere.

Previously (in 1.x), it was res:/, and it is now builtin .... Unfortunately, I don't like this, it seems quite awkward and not intuitive.

Respecting an idea of DSL, we would find templateFile = builtin("testcase/testcases.tmpl"), builtin:/... or something similar better. Well, although I doubt typesafe-config support this, I suggest this configuration schema could be modified before public release.

What do you guys think?

zen0wu commented 10 years ago

The reason I change it from res:// is only because res is a bad name and :// seems ugly and not consistent with the overall style of the config file.

The typesafe-config does not support these, as I stated in the other thread, the library is not strong enough. I tried to use builtin(...) (without double quotation), seems working, the only drawback is that we have to parse the value and get rid of the parentheses manually, but that's not a serious problem.

@vexorian What do you think?

vexorian commented 10 years ago

I liked res://. I've seen it used in other programs. I don't think it is inconsistent with the config file. The string it expects is a path. And res://xxx/yy is a path. Whereas builtin xxx/yy is not a path, so it adds more syntax. Is res a bad name? Maybe you could use greed:// or jar:// or builtin:// .

Regarding builtin(...) vs builtin ... both are equally bad to me :)

vexorian commented 10 years ago

I am leading towards builtin() if something:// is not possible. Also, if we want consistency, then I think ${somevariablename}/fire_path_in_resources could work?

wookayin commented 10 years ago

I found the groovy-style DSL very elegant (see build.gradle for example). Since the configuration engine itself is not a groovy, we can't handle a complex syntax as above, and although maybe groovy is a overkill for the plugin -- but I think it should be greatly encouraged to conform to its syntax as long as we can.

Anyway, if builtin(..) is impossible, I think we should go back in the way such as res:/ or builtin:/. I don't think this is inconsistent -- see Page "URI Schemes" on wikipedia.. The styles like 'classpath:/path/to/res' or 'res:/' seem to be commonly used in other projects, too.

zen0wu commented 10 years ago

I'm totally with you on the groovy thing. Groovy is very powerful on defining DSL with a very concise and elegant syntax. But it's too heavy to bring it into this small toolkit, that's why I'm also considering reimplement a template engine with a config library which can be fully compatible and complementary of each other. But this is a whole big piece of work which may take even more effort than Greed itself.

The reason I ruled out the name res is this. res came into use in the first place is because the old folder structure in greed is just a Resource folder and all things are put into it, and res stands for Resource. As to 2.0, we have a lot of new templates defined instead of just a few, and the folder structure get a lot more complicated. Instead of just "Resource", we want to express explicitly, these are "builtin template". I always stick to the principle that the user side should stay as intuitive and precise as possible, to make the user know what they're doing just by the name. And in the future, maybe, we may have more things other than templates, like scripts, or anything. A single res may get user confused.

I like the syntax of builtin(), and it is possible, as I stated before, with some workaround. But I'm starting to think builtin may not be a good name. builtin-template() maybe? I don't know, it's too long.