Configuration for formatter

gavinking commented 10 years ago

We need a way to configure the formatter.

Config needs to be per-project.
It needs to be persisted in a git-friendly form and needs to be editable as plain text.

So a properties file type thing would be a great start, though I suppose we will eventually want a GUI for it.

lucaswerkmeister commented 10 years ago

The formatter already reads the .ceylon/config file and uses settings from the “formatter” section (ceylon/ceylon.formatter#32), so we just need to add a nice UI for it.

(You can also read other config files, so we could use an IDE-specific config… but I really don’t see the point in that.)

akberc commented 10 years ago

Approach:

Project configuration with optional workspace settings (future?)
Implement IPersistentPreferenceStore over Ceylon config, with scope on config file section formatting. The other option is to use the CeylonProjectConfig but that is geared towards the compiler and builder and not for individual option maps. Also, using an IPersistentPreferenceStore implementation allows us to store global config in Eclipse workspace preferences.
Only store options that are changed from default values.

We will need to group the options into sections that show on different tabs.

lucaswerkmeister commented 10 years ago

You can also put global configuration into the user Ceylon config file - IIRC it cascades just like Git config: options from the project config file override options from the user config file.

In fact, AFAIK, you can put a config file on any directory level, so you could get a workspace-specific config in $workspace/.ceylon/config. But I haven't tested that.

(Sent from phone, sorry for the lack of formatting.)

gavinking commented 10 years ago

However you do it, I would like Eclipse to store its formatter prefs in some place where the commandline formatter can see them.

quintesse commented 10 years ago

In fact, AFAIK, you can put a config file on any directory level, so you could get a workspace-specific config in $workspace/.ceylon/config.

Well you could, but that would only work when your projects are subfolders (at some level) inside the workspace. If that's not the case the command line version would not know about it.

I do think that if both Eclipse and the CLI version need to use the same settings that the only real option we have is using our own config file. And in that case I also think that it should be the same format we already use for the configuration.

But my suggestion would be to just use a separate file within the Ceylon folder, let's say .ceylon/format or .ceylon/style.

I'm not certain how to deal with the workspace settings though. I think the only solution would be to always copy the workspace settings to the format config file and apply the project settings on top of them. The problem is to figure out afterwards which settings were made as part of the project and which were part of the workspace.

None of the things I can come up with will win any beauty prizes. We could either store them as a separate Eclipse specific section in the file, a section that gets ignored by the CLI tool. Or we could store them in a separate file .ceylon/format.eclipse. Or just use Eclipse's preferences settings.

lucaswerkmeister commented 10 years ago

Well you could, but that would only work when your projects are subfolders (at some level) inside the workspace. If that's not the case the command line version would not know about it.

Damn, you’re right, that’s not necessarily the case.

But my suggestion would be to just use a separate file within the Ceylon folder, let's say .ceylon/format or .ceylon/style.

Why not just a [formatter] section in the regular config file, as it’s already implemented? (Also, I can’t find how to pass a file name to CeylonConfig.get, but I might just be overlooking something.)

gavinking commented 10 years ago

Since all the infrastructure for the config file is already implemented, I don't see why it shouldn't go in there.

FroMage commented 10 years ago

Well, for configuration syncing there's a conflict of locations where it makes sense to sync:

Project-local config
User-global config (only Ceylon)
Workspace-local config (only Eclipse)

Let's not pretend that the Ceylon CLI can read config from a workspace, this is not a notion that exists on the File System.

For project-specific settings there's no question. For the rest, what we can do is either:

Just store the workspace settings in the workspace and the CLI will not be able to use it
Store the workspace settings in the user config with a big fat warning and both can use it, but it's not workspace-specific

gavinking commented 10 years ago

Store the workspace settings in the user config with a big fat warning and both can use it, but it's not workspace-specific

Honestly this sounds fine to me.

quintesse commented 10 years ago

Why not just a [formatter] section in the regular config file, as it’s already implemented?

First of all, there's almost no work involved, the code doesn't care from which file the information comes.

So why not in that file? Because to me the config file is mostly meant for required things. Now this is not a hard and fast rule but most of its settings are about things that if you don't set them things don't work,

The formatting on the other hand is completely personal, to be honest I do not want your blooming formatting settings in my project!

So a team of people might just say: "please add your format file to your .gitignore, we don't want one person's settings to conflict with somebody else's".

If we put them in the normal config file suddenly people can't even share their important setting s anymore.

That's why I think it should be in a separate file, at least.

lucaswerkmeister commented 10 years ago

Fine with me too. We can just call them "user-specific" and forget that they're workspace-specific in JDT :)

----- Ursprüngliche Nachricht ----- Von: "Gavin King" notifications@github.com Gesendet: ‎17.‎07.‎2014 12:00 An: "ceylon/ceylon-ide-eclipse" ceylon-ide-eclipse@noreply.github.com Cc: "Lucas Werkmeister" mail@lucaswerkmeister.de Betreff: Re: [ceylon-ide-eclipse] Configuration for formatter (#987)

Store the workspace settings in the user config with a big fat warning and both can use it, but it's not workspace-specific Honestly this sounds fine to me. — Reply to this email directly or view it on GitHub.

FroMage commented 10 years ago

I don't know about user-project specificity. It depends on several things:

Does Eclipse store the JDT formatting rules in the generic .project file?
Do projects store their Eclipse .project in their VCS?
Do projects store their Ceylon .ceylon/config in their VCS?
Do we want to introduce a convention of .ceylon/config being stored in VCS while .ceylon/user-config not being stored there?

IMO if you have a project formatting settings, it should apply to the whole project, and override any user setting for formatting.

gavinking commented 10 years ago

The formatting on the other hand is completely personal, to be honest I do not want your blooming formatting settings in my project!

I see how that's correct. There should be one canonical set of formatting standards for the project. The whole point of a formatter is to avoid "format wars".

quintesse commented 10 years ago

There should be one canonical set of formatting standards for the project

Let's just let people decide that for themselves, shall we? If and when they want that they can do so and they can just add the "format" file to Git and things will work just fine, and if they don't want that they just leave it out. But adding it to the "config" file takes away that choice.

tombentley commented 10 years ago

imo it's far more likely people will just add the whole .ceylon directory or not, as a whole. That's what I'd do with the eclipse .settings director, for example, because I have no idea if any of the files in there are interdependent. In other words, for that to really be a choice you need to be documenting the contents of that directory, so people know that adding or not an individual file will have the effect they intend without nasty side-effects.

quintesse commented 10 years ago

Well, I'm not sure if there are any nasty side effects, but yes it should be documented of course. We already do this btw : http://www.ceylon-lang.org/documentation/1.0/reference/tool/config/

Btw, another good reason IMHO to use separate files is that you can easily copy them around. You could have different "formatting profiles" lying around and the only thing you'd have to do to apply it to another project is to copy it. On top of that in the CLI tool you could easily point to a different one saying "format the code with this particular style".

quintesse commented 10 years ago

Btw, I just made some changes to the Config API that will make using a separate file even simpler. Before the CeylonParser would have a bunch of loadConfigXxx() and findConfigXxx() methods and besides not being a proper place for those methods they were hard-coded to just look for a config file. Now, you could load whatever file you want with the Config API, but the lookup code that handles finding and merging configuration files is useful in other cases as well, so I now split it out into a new class ConfigFinder and made it more configurable. You can take a look at CeylonConfigFinder to see how easy it would be (a one-line change) to create one for a differently named file.

lucaswerkmeister commented 10 years ago

I’m inclined to agree with @quintesse. Even though I’ve previously argued that having user-specific formatting settings is evil, his arguments make a lot of sense :)

But then I remembered that the formatter also supports “include” configuration with include=fileName (both in config files and in command line arguments). With this, you could also create @quintesse’s workflow by adding formatter.include=.config/formatter to the config file. (Untested – not sure what the path needs to be relative to, especially from the IDE.)

This also allows you to keep different “formatting profiles” – e. g. in ~/.ceylon/formatter/ – and change which one you include on the fly. (And they can cascade! include=/etc/ceylon/companyWideFormatterBaseSettings or something like that.)

And now I’m not sure if it’s better to read from a specific config file by default.

@quintesse does the new Config API support something like “look in .ceylon/formatter, then .ceylon/config, then ~/.ceylon/formatter, then ~/.ceylon/config etc.”? (Or does that sound crazy to you? I’m really not sure how I feel about it.)

quintesse commented 10 years ago

And they can cascade!

I'd not use includes, they are also notoriously fragile across multiple OSes, you show include=/etc/ceylon/companyWideFormatterBaseSettings which for example won't work for our team where all 3 supported OSes are represented.

I too would think more along the lines of having "profiles", stored in ..../.ceylon/formatter/* which we could find using the same rules we use for finding ..../.ceylon/config (which means they would automatically be found in ~/.ceylon/ and /etc/ceylon or their alternatives on other OSes). We might even add an extra search path for the distribution folder so we could add default formatter profiles to the installation itself.

Then if you run the formatter without any profile name, and none its otherwise specified, it could look for default. Or you specify an explicit profile name and it will look for that file. (First in .ceylon/formatter/xxx, then up the hierarchy and finally trying ~/.ceylon/formatter/xxx, /etc/ceylon/formatter/xxx and $CEYLON_HOME/defaults/formatter/xxx)

@lucaswerkmeister right now the API supports only looking into a single file. Although that could possibly be changed. But yes, I think it's a bit crazy :)

If we'd go for this idea of profiles we could get rid of this .ceylon/format idea of mine because the configuration would be a single line, like formatter.profile=company-defaults.

Point is I'm still not sure how I feel about having it in .ceylon/config (but at least it would be very easy to change and having a special .ceylon/format file for a single line doesn't make sense), to me the whole idea is that there is a reason why formatters in IDEs have so many options and why nobody adds those preferences to Git. But right now I can't think of anything better than what I stated above.

akberc commented 10 years ago

Great points. To summarize:

Formatting preferences are a user's preferences and may not be desirable at a project level
At the moment, ceylon.formatter reads them the 'formatter' section in .ceylon/config
A user's global preferences in file named 'style' would be very useful in ~/.ceylon
Eclipse workspace storage in the workspace .metadata is not very useful.
Ceylon config tool's Git-like hierarchy is preferred.

As such,

will support current formatter functionality, reading and overriding, in order -
- built-in,
- user folder .ceylon/style file, formatter section,
- project config file, formatter section
would allow saving of profiles to arbitrary locations

Do we need to read or store a formatter version in the settings? Formatting options are done once in a while and then almost forgotten. Any changes to default built-in settings in the future will cause annoyance. With a settings version, it will give future versions of the formatter a chance to adapt current settings to new defaults.

quintesse commented 10 years ago

Do we need to read or store a formatter version in the settings

I think the best thing to do is to not leave things open to interpretation and always write all the formatting options to the file. So unlike preferences that are merged, styles would always be complete (which is also how Eclipse works, you don't just set single style options on a project, you either take the workspace settings or you choose a completely different style, even if that style differs by just a single setting).

That way if a new formatter is released with new defaults you will still have your old behavior. And any new settings will just get their default value because they would be missing from the style file.

gavinking commented 10 years ago

@akberc are you actively working on this? We plan a release in a month, and it would be nice to have this in. WDYT?

akberc commented 10 years ago

Yes, definitely. I got a bit too ambitious -- with preview and everything -- while trying to avoid Eclipse internal packages. Will cut down and have a working version in this weekend. Just need to verify:

There is no need for a code branch, as the only integration point so far is plugin.xml propertyPages extension point.
To follow the Java pattern, the formatting options will be a sub-page in the 'Ceylon Code Style' group. So, we need something to put in the main tab. One thing I thought of was the initial module version when creating a module: '1.0.0' or '0.1'. Any other ideas? Or drop this thought, and make the tab 'Ceylon Code Formatting' as the only page?

gavinking commented 10 years ago

@akberc I would just make it a subpage under General > Edtors > Ceylon Editor. I anyway need to split some of the stuff out of Ceylon Editor and move it to one or more subpages.

gavinking commented 10 years ago

Oh wait, this is a project properties page, is that what we decided? Hrm.

Perhaps we should rename the Ceylon Compiler page to just Ceylon, and stick it under there?

Or yeah, perhaps you're right, perhaps a Ceylon Code Style section.

luolong commented 10 years ago

It always made me wonder -- doesn't Ceylon deserve a root preference node of it's own?

I mean, I would not know to go and search for the Ceylon preferences under the General > Editors section if I did not read it from here...

lucaswerkmeister commented 10 years ago

Update: The formatter is now configured through “profiles”, which are config files named "format.``profile``" (profile defaults to "default"). The IDE should use loadProfile { profile = ...; inherit = false; } and always write out a full config file (I’ll probably add some utility for this).

(EDIT: configOptions() was renamed to loadProfile())

lucaswerkmeister commented 10 years ago

Added saveProfile() to save a full profile.

akberc commented 10 years ago

Sorry for the delay in this - a day-job project that turned out to be a day-night-weekend project.

Here is the code that is standing up now, but needs about 8-10 hours to be good enough to be pushed, and another 8 hours or so to add all the base options : https://github.com/akberc/ceylon-ide-formatter-options
@lucaswerkmeister will look at the profile save. Right now, I am assuming one project-specific style file that can override the built-in profile. Continuing to work on it, and hope it can still make 1.1

lucaswerkmeister commented 10 years ago

Hey @akberc, sorry it took me so long to respond. I looked at your code, and I’m a bit worried about CeylonCodeFormatterConstants. It seems like lots of duplication; do you think it could be removed if we added some more annotations to FormattingOptions instead, which you could read using reflection / the metamodel? (Perhaps not…)

Also, can I test this somehow? I tried copying the files into my local repository, but it didn’t work.

lucaswerkmeister commented 10 years ago

Oh, and thanks for your work! I couldn’t test it, but I see a file called CompilationUnitPreview.java, which if I’m guessing correctly sounds pretty neat.

davidfestal commented 10 years ago

@lucaswerkmeister : any idea about whether you will be able to test /validate/merge this before 1.1 ?

gavinking commented 10 years ago

@lucaswerkmeister @akberc Status, please?

akberc commented 10 years ago

@lucaswerkmeister I have it working with formatter profiles and using loadProfile and saveProfile. Regarding annotations, I have created a wrapper to map preferences as displayed to FormatterPreferences Ceylon class, and the wrapper can use metamodel later.

@gavinking checking in working version today.

lucaswerkmeister commented 10 years ago

That sounds great, especially the wrapper. I look forward to checking it out tomorrow! :)

----- Ursprüngliche Nachricht ----- Von: "Akber Choudhry" notifications@github.com Gesendet: ‎21.‎09.‎2014 18:49 An: "ceylon/ceylon-ide-eclipse" ceylon-ide-eclipse@noreply.github.com Cc: "Lucas Werkmeister" mail@lucaswerkmeister.de Betreff: Re: [ceylon-ide-eclipse] Configuration for formatter (#987)

@lucaswerkmeister I have it working with formatter profiles and using loadProfile and saveProfile. Regarding annotations, I have created a wrapper to map preferences as displayed to FormatterPreferences Ceylon class, and the wrapper can use metamodel later. @gavinking checking in working version today. — Reply to this email directly or view it on GitHub.=

akberc commented 10 years ago

Mostly working - checked into branch - https://github.com/ceylon/ceylon-ide-eclipse/tree/formatter-profiles

Remaining

map all options
implement put without access to VariableOptions
preview works but preview sample needs to be more representative

Issues and Feedback Requested

Added a parameter to saveProfile to accommodate profile export. Pull request https://github.com/ceylon/ceylon.formatter/pull/82
Formatter use prefix format. for profiles, and ConfigWriter appends .old, so the profiles keep increasing
Import imports from an arbitrary file to the project .config folder, renaming it to format.{profileName}. Obviously we would want to import to different scopes
Hooks put into FormatAction to pull active profile and into NewModuleWizard for pulling in a sample non-formatter style parameter - new module version. This tab can be taken out and only the formatter tab letft, but I think it is useful as many templates can use general non-formatter style options.
Using .ceylon/style to store style values and a pointer to the active profile. Need feedback on whether a project-specific profile should be saved in this file, or leave the profiles as is, and merge the [style] section inside the main .ceylon/config
format is a bit too slow for the preview function
if preview sample has any imports, the CPC fails and formatter fails as the sample code in the preview is being formatted without reference to any module or package. Perhaps this will be so until ceylon.ast.

quintesse commented 10 years ago

Formatter use prefix format. for profiles, and ConfigWriter appends .old, so the profiles keep increasing

What's this problem exactly?

as many templates can use general non-formatter style options.

What are examples of non-formatter style options? (to be able to say anything about the next point in the list)

akberc commented 10 years ago

saveProfile in the formatter module uses the format format.{profileName} to store the config file for the profile -- prefix instead of suffix. Ceylon config adds .old as the suffix. So a profile with the name format.team gets backed up as format.team.old. This shows up as another profile (maybe I can filter it out) and if modified, the backup is saved as format.team.old.old. We can either filter out the 'old' files, or if there is an option in config to not make backups, we can use.
I added 'new module version' and 'new module author' (not hooked up) . So, maybe things like 'license', 'make an internal package' and other repetitive things that. And importantly, upper- and lower-case conventions for quick fixes and other templated code.

@lucaswerkmeister The Sparse... constructor is huge for just adding one option at a time, which the JDT-borrowed framework does. Alternate is to have another backing model and update Sparse... in one go on save. Is there a way to expose VariableOptions?

I will not be able to work on this for a day and a half. I have added one more option and pushed it to the branch. It is just now adding options and tabs one by one. And of course, fixing bugs :)

quintesse commented 10 years ago

ok, if people think backups for profiles are not necessary I can add an option probably to disable that. If so please add an issue in ceylon-common for that.
but are those options supported by the command line formatter?

lucaswerkmeister commented 10 years ago

Formatter use prefix format. for profiles, and ConfigWriter appends .old, so the profiles keep increasing Ceylon config adds .old as the suffix. So a profile with the name format.team gets backed up as format.team.old. This shows up as another profile

Interesting problem, I hadn’t thought of that. It’s probably best to swap the name parts – other tools use name suffixes as well (format.default~), format is a nice FNE, and default.format sounds nicely like “the default format”. @quintesse any objections?

The Sparse... constructor is huge for just adding one option at a time, which the JDT-borrowed framework does. Alternate is to have another backing model and update Sparse... in one go on save. Is there a way to expose VariableOptions?

I will not expose VariableOptions – formatting options should never be able to change during formatting. What I could do is make the members of SparseFormattingOptions variable, and change CombinedOptions to be an eager copy instead of a lazy wrapper, so that later changes to the SparseFormattingOptions wouldn’t propagate through the CombinedOptions. Would that help you?

format is a bit too slow for the preview function

ceylon/ceylon.formatter#34 :) And I haven’t looked at it in depth yet, but I hope you’re not re-lexing and -parsing the preview code each time? That could be part of it.

if preview sample has any imports, the CPC fails and formatter fails as the sample code in the preview is being formatted without reference to any module or package. Perhaps this will be so until ceylon.ast.

What do you mean? Are you trying to fully compile the sample? We don’t need that, we only need lexing + parsing, no typechecking. See here.

quintesse commented 10 years ago

any objections?

none whatsoever.

akberc commented 10 years ago

@quintesse No, these style options are used by other functions, for example, the new project wizard uses the 'new module version' instead of just using '1.0.0'. I found myself changing it every time I added it.

Style is more than just formatting. So, I think we do need a main 'style' page, and then a sub-page formatter. If not, we can put the formatter page on the top level in project properties. Java also separates the compiler options from the style options. Let me know what you think.

quintesse commented 10 years ago

@akberc It's not that. It's just that only options that are part of the CLI tools should go to Ceylon config files (or format files). Anything that's IDE related should just use the normal Eclipse preferences stuff.

akberc commented 10 years ago

@lucaswerkmeister - thank you. Pls change the SparseFormattingOptions and I will lex the sample once. Oversight :)

@quintesse We will keep the style file and ensure that all parameters are potentially usable by the CLI.

lucaswerkmeister commented 10 years ago

Okay, I played a little with the formatting options, and it turns out I didn’t really think it through:

If I make the attributes variable in SparseFormattingOptions, they have to be variable in FormattingOptions as well, because FormattingOptions extends SparseFormattingOptions.
FormattingOptions isn’t final, and in fact the attributes are even default, so that at the moment you could actually go ahead and create your own VariableOptions class. Not good!

So the current plan is now:

SparseFormattingOptions stays as it is
VariableOptions extends SparseFormattingOptions and is shared
FormattingOptions extends SparseFormattingOptions and declares its attributes not default.

lucaswerkmeister commented 9 years ago

I just saw that you’re

Using .ceylon/style to store style values and a pointer to the active profile.

The CLI formatter has a different config for that: formattool.profile in the main config file (see function configProfileName()). You should probably use that too.

gavinking commented 9 years ago

I take it this is for 1.2.

lucaswerkmeister commented 9 years ago

I think we should be able to finish this in time for 1.1, it looks good to me so far. @akberc do you have any more unpushed work? If you’re busy with your other project, I can continue expanding FormatterPreferences and FormatterConstants – but of course it would be better if I don’t redo a lot of stuff that you already have locally :)

lucaswerkmeister commented 9 years ago

format is a bit too slow for the preview function

[…] I hope you’re not re-lexing and -parsing the preview code each time? That could be part of it.

0b7491373e6f43ee4c4cb1d478ce7673ceed12c6. It’s almost instant now :)

akberc commented 9 years ago

Rest will be pushed today. Two more tabs. That's it.

Akber Choudhry Digiwave Systems Ltd. On 30 Sep 2014 18:00, "Lucas Werkmeister" notifications@github.com wrote:

format is a bit too slow for the preview function

[…] I hope you’re not re-lexing and -parsing the preview code each time? That could be part of it.

0b74913 https://github.com/ceylon/ceylon-ide-eclipse/commit/0b7491373e6f43ee4c4cb1d478ce7673ceed12c6. It’s almost instant now :)

— Reply to this email directly or view it on GitHub https://github.com/ceylon/ceylon-ide-eclipse/issues/987#issuecomment-57390037 .

eclipse-archived / ceylon-ide-eclipse

Configuration for formatter #987

Remaining

Issues and Feedback Requested