Community
Participate
Working Groups
This PR is for keeping track of and comment the renaming of the code formatter options. Intermediate versions of the file can be attached here for revision by anyone interested.
Proposition: By making the difference between 'arguments' (calls) and 'parameters' (declarations), we can omit INVOCATION and DECLARATION for the respective options. Example: FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_INVOCATION_ARGUMENTS becomes FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_ARGUMENTS
Created attachment 7409 [details] Options prior to renaming. The options have been reorganize by topics. It makes it simpler to find a option in the list. Then we can discuss the common pattern names.
The 'INSERT_SPACE' bunch of constants could be better grouped, but that's easier once the options are named in a more consistent manner. I'd like to do the grouping as soon as the names are changed, if you want. I've already grouped these options once in a (hopefully) semantic consistent way on my pref page, so I can bring in the thoughts I made there. The deprecated options should be put together somewhere at the end of the file, its rather confusing having them mixed with the other options.
Ok, I will move all deprecated at the end of the file. Now let's see the renaming. 1) Changing all INSERT_SPACE to match this pattern INSERT_SPACE_{BEFORE/AFTER}_{SYNTACTICAL ELEMENT}_IN_{JAVA LANGUAGE ELEMENT} 2) Changing all BRACE_POSITION to match this pattern: FORMATTER_BRACE_POSITION_FOR_{JAVA LANGUAGE ELEMENT} 3) Changing all ALIGNMENT for FORMATTER_ALIGNMENT_FOR_{SYNTACTICAL ELEMENT}_IN_{JAVA LANGUAGE ELEMENT} What do you think?
Ok, that's fine. We also need: INSERT_SPACE_BETWEEN_EMTPY_{PAREN/BRACES/BRACKETS}_IN_{JAVA ELEMENT} INSERT_SPACE_AROUND_{SWITCH_CONDITION/IF_CONDITION/...}
Applying comment 1, we end up in a collision case. The constant would not be used for the same option anymore. FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_ARGUMENTS => FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_PARAMETERS FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_MESSAGESEND_ARGUMENTS => FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_ARGUMENTS Can this be handled in the conversion layout?
Created attachment 7429 [details] New version after renaming Silvio, could you please review the constants after renaming? If this is fine for you, I will update all references to the old constants in JDT/Core code. I left some TODO tasks to remember that I have to add three new options. Let me know if you think I forgot something. All deprecated constants are left at the end of the file with its new corresponding option(s).
Regarding comment 1, I removed the DECLARATION, but I left INVOCATION to prevent duplicate constants. Let me know if this is fine for you.
Created attachment 7433 [details] My changes, grep for TODO Hmm, looks good! I like that you got rid of the INSERT_SPACE_IN_FOR_PARENS etc by providing options to configure space after opening and before closing paren seperately! This greatly facilitates my new ordering scheme which focusses on syntactical elements [](){},; etc. Regarding my comment 1, I haven't considered the fact that for example BEFORE_OPENING_PAREN_IN_METHOD doesn't include either the word ARGUMENT or PARAMETER. So I think for consistency reasons, we should add _DECLARATION whenever the corresponding option for declarations contains _INVOCATION. As a general rule, the effect of an option should be unambiguously described by its name. The duplicate constant is not so much of a problem, I can handle this in my compatibility code. But would it be possible to assign a new value to the re-used constant? By appending some number to the string for example? Else its difficult. I made some comments in the file, look out for the TODO tags. I also removed several of the deprecated alignment options, which have never been used in my code and consequently don't need to be preserved. What about type_member_alignment? I doesn't seem to fit into the ALIGNMENT options as actually it's more kind of a boolean option: multicolumns or not. Therefore, I propose to take it out of the ALIGNMENT options (by renaming it) and make it boolean. And I discovered an alignment options which is already present in the old constants but which I've never used so far: MULTIPLE_FIELDS_ALIGNMENT. What does it do? Is it a standard line wrapping option? There's also a new one, FORMATTER_ALIGNMENT_FOR_PARAMETERS_IN_CONSTRUCTOR. I don't currently have a good overview about what's new compared to the version we've used in this weeks integration build. Can you give me some summary? Further, given the constaint of using string values and not having available native enumerations, the value ranges and types for each option (or at least the uncommon ones) should be specified in javadoc. Especially the alignment options are quite confusing, as the API is really low level. Internally for the formatter page, I use a wrapper class which is used to set the alignment options and convert them to value string, maybe we can move this to core? Bit-shifting and masking should be hidden behind an API... ;-) But that's another issue, I think I'll make a new PR to discuss this. Would be a good opportunity now to change this together with the renaming, as all ALIGNMENT option names change anyway.
Let's try to answer all your questions: I will add again _DECLARATION to be consistent. Then we should not have any conflicts or duplicates in the naming. The suggestion for type_member_alignment is good. Let's make it a boolean. The new name could be: FORMATTER_ALIGN_TYPE_MEMBERS_ON_COLUMNS value TRUE/FALSE The MULTIPLE_FIELDS_ALIGNMENT is a way to wrap multi field alignments. There is no equivalent for multiple local declarations because it would be too costly. This will be added once we are using DOM/AST nodes. FORMATTER_ALIGNMENT_FOR_PARAMETERS_IN_CONSTRUCTOR is the same than the ones for the methods. The constructor alignment was reusing the method alignment. This was not consistent with other options for constructors and methods. Hopefully I answered all your questions. I will use your version and implements all the TODOs. I will attach it again once it is done and let me know if this is fine. I'd like to do all other changes today and then we could release the code tomorrow for Friday's nightly build. What do you think?
I only meant to add _DECLARATION where the name would be ambiguous, for example in FORMATTER_INSERT_SPACE_AFTER_OPENING_PAREN_IN_METHOD (decl or invocation?). But for the duplicate you mentioned (FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_ARGUMENTS), nothing changes and we still have the name clash. Would it be ok to assign another string value to this constant? This would solve everything (for example, appending '2' or so). Sorry for the long comment 9, I was just hacking and writing at the same time.
Created attachment 7437 [details] Last version. Please review it. I'd like to make a patch with this version.
Created attachment 7440 [details] New version Ok, here's another revision. I've grouped some things and added some TODOs.
FORMATTER_INSERT_SPACE_AFTER_CLOSING_BRACE_IN_BLOCK is missing and should be added.
FORMATTER_INSERT_SPACE_BEFORE_FIRST_PARAMETER_IN_CONSTRUCTOR FORMATTER_INSERT_SPACE_BEFORE_FIRST_PARAMETER_IN_METHOD should rather be named FORMATTER_INSERT_SPACE_AFTER_OPENING_PAREN_IN_CONSTRUCTOR_DECLARATION FORMATTER_INSERT_SPACE_AFTER_OPENING_PAREN_IN_METHOD_DECLARATION
FORMATTER_INSERT_SPACE_BEFORE_CLOSING_PAREN_IN_CONSTRUCTOR -> FORMATTER_INSERT_SPACE_BEFORE_CLOSING_PAREN_IN_CONSTRUCTOR_DECLARATION
Another name clash: ORMATTER_INSERT_SPACE_BEFORE_COMMA_IN_METHOD_ARGUMENTS FORMATTER_INSERT_SPACE_AFTER_COMMA_IN_METHOD_ARGUMENTS used to affect method declarations, now they are used for invocations... Hmmm, let's add _invocation and _declaration everywhere. Ok?
FORMATTER_INSERT_SPACE_BEFORE_FIRST_EXPRESSION_IN_ARRAY_INITIALIZER -> FORMATTER_INSERT_SPACE_AFTER_OPENING_BRACE_IN_ARRAY_INITIALIZER
I've just finished double-checking all the insert_space options, and I'm quite happy about the naming! Sorry for the mass of comments though...at least they're small... ;-)
Would you have a final version around?
Created attachment 7448 [details] Last version Hopefully this is it. I am about to update the code formatter with this version. Is it ok for you? I think I update it with all your last changes. This constant is now a boolean FORMATTER_ALIGN_TYPE_MEMBERS_ON_COLUMNS. I didn't get your point about the BLANK_LINES_AT_BEGINNING_OF_METHOD_BODY. let me know before I arrive tomorrow morning if this is ok. If yes, I will work on a patch for you.
* We should unify on either using CONSTRUCTOR or CONSTRUCTOR_DECLARATION thoroughly. The former requires less options to change, the latter is more consistent with METHOD_DECLARATION. * Using reference search, I checked all the deprecated constants on whether they appear in jdt.ui code or not. Here's a list of constants which have never been used, either because I forgot to add them or because you've added them only recently after last I-build. These could be removed safely without deprecating them first from my point of view: FORMATTER_MULTIPLE_FIELDS_ALIGNMENT FORMATTER_INSERT_SPACE_BEFORE_CONSTRUCTOR_DECLARATION_FIRST_ARGUMENT FORMATTER_INSERT_SPACE_BETWEEN_CONSTRUCTOR_DECLARATION_EMPTY_ARGUMENTS FORMATTER_INSERT_SPACE_BEFORE_CONSTRUCTOR_DECLARATION_CLOSING_PAREN FORMATTER_INSERT_SPACE_BEFORE_CONSTRUCTOR_DECLARATION_FIRST_PARAMETER FORMATTER_INSERT_SPACE_BEFORE_CONSTRUCTOR_DECLARATION_OPEN_BRACE FORMATTER_INSERT_SPACE_BEFORE_CONSTRUCTOR_DECLARATION_OPEN_BRACE FORMATTER_INSERT_SPACE_BEFORE_METHOD_INVOCATION FORMATTER_INSERT_SPACE_BEFORE_METHOD_DECLARATION_CLOSING_PAREN FORMATTER_INSERT_SPACE_BEFORE_METHOD_DECLARATION_FIRST_PARAMETER FORMATTER_NUMBER_OF_BLANK_LINES_TO_INSERT_AT_BEGINNING_OF_METHOD_BODY FORMATTER_ALIGNMENT_FOR_TYPE_MEMBERS FORMATTER_NUMBER_OF_BLANK_LINES_AT_BEGINNING_OF_METHOD_BODY * Why did you rename FORMATTER_INSERT_SPACE_BEFORE/AFTER_COMMA_IN_METHOD_THROWS into FORMATTER_INSERT_SPACE_BEFORE/AFTER_COMMA_IN_THROWS_CLAUSE We have throws clauses also for constructors (FORMATTER_INSERT_SPACE_BEFORE/AFTER_COMMA_IN_CONSTRUCTOR_THROWS), so it's not consistent anymore. I'd leave it as they are or to call them FORMATTER_INSERT_SPACE_BEFORE/AFTER_COMMA_IN_METHOD_DECLARATION_THROWS FORMATTER_INSERT_SPACE_BEFORE/AFTER_COMMA_IN_CONSTRUCTOR_DECLARATION_THROWS. What do you think?
My last point in comment 22 depends of course on whether we agree on CONSTRUCTOR or CONSTRUCTOR_DECLARATION. I'd prefer the latter, so constructors and methods are treated everywhere in the same way. Hoping that these we're the last changes...
Ok, I will make the last changes. I send you the file again for review and then I update the code formatter.
Created attachment 7456 [details] Here we go. The last one with all your last changes Please, have a look. I think this is fine now. I removed unused constants and change CONSTRUCTOR for CONSTRUCTOR_DECLARATION everywhere. I also fixed the THROWS constants. Let me know what you think. If you are ok with this one, I update the code formatter and hopefully I can provide you a patch later today.
I also added: FORMATTER_ALIGNMENT_FOR_THROWS_CLAUSE_IN_CONSTRUCTOR_DECLARATION FORMATTER_BRACE_POSITION_FOR_CONSTRUCTOR_DECLARATION
Just two issues: * You took out FORMATTER_ALIGNMENT_FOR_TYPE_MEMBERS instead of FORMATTER_TYPE_MEMBER_ALIGNMENT. * FORMATTER_INSERT_SPACE_BEFORE_OPENING_BRACE_IN_METHOD misses the _DECLARATION suffix. I think everything else is fine, I'm looking forward to your patch.
Created attachment 7461 [details] Fixed. Here is the latest version I should be able to issue a patch later today.
I also added: FORMATTER_INSERT_SPACE_BEFORE_OPENING_BRACKET_IN_ARRAY_ALLOCATION_EXPRESSION
FORMATTER_INSERT_SPACE_BETWEEN_BRACKETS_IN_ARRAY_TYPE_REFERENCE is not deprecated anymore, because for an array type reference the brackets are always empty. So there is no need to add EMPTY in the name. This is redundant and not consistent with other settings that make the distinction between empty and not empty.
Please, check that you are using all constants in the code formatter preference page. I checked on my side that the formatter is using each constant. All tests are passing with the new version.
Set milestone
Fixed and released in HEAD.
Verified for 3.0-M7 with build I200402102000.
