Skip to content
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

Feature Request: Missing spaces #20

Closed
jelliottp opened this issue May 24, 2023 · 10 comments
Closed

Feature Request: Missing spaces #20

jelliottp opened this issue May 24, 2023 · 10 comments
Assignees

Comments

@jelliottp
Copy link

jelliottp commented May 24, 2023

Similar to existing space rules, there should be rule (or perhaps include in "Close brackets at line end"?), where a space is expected between a ' and ( or ) for method calls and inside VALUE statements.

Before:

cl_abap_unit_assert=>assert_not_initial('some-string').

After:

cl_abap_unit_assert=>assert_not_initial( 'some-string' ).

VALUE Before:

var = VALUE #( ( field = '12345')
               ( field = '67890') ).

VALUE After:

var = VALUE #( ( field = '12345' )
               ( field = '67890' ) ).
@jmgrassau
Copy link
Member

Hi Josh,

oh, that's an excellent idea, will do! I never understood why ABAP syntax even allows this…

Kind regards,
Jörg-Michael

@jmgrassau
Copy link
Member

P.S.: Hm, it doesn't really fit into any of the existing rules, so I think a dedicated rule would make sense.

@jelliottp
Copy link
Author

jelliottp commented May 24, 2023

Hi Josh,

oh, that's an excellent idea, will do! I never understood why ABAP syntax even allows this…

Kind regards, Jörg-Michael

I agree, it should be a syntax error. Also, I edited above, the same issue applies to ( and '.

@ConjuringCoffee
Copy link
Contributor

Please note that it also applies to the backtick ` and not just '.

@jmgrassau
Copy link
Member

Hi Josh and @ConjuringCoffee,

well, in the end I thought that there was always some lack of complexity to the cleanup rule "Remove multiple spaces in empty parentheses" :-), so in the next release, that rule will be renamed to "Standardize spaces next to parentheses" and enhanced as follows:

image

image

Activating the third option will additionally change lines 8, 22-24, and 32:

image

image

Hope that works. As lines 27-29 show, some care was needed to not mess up cases with dynamic (method, component, attribute, …) names.

Kind regards,
Jörg-Michael

@jmgrassau
Copy link
Member

P.S.: Hm, the wording is a bit difficult, maybe it's better like this:

image

@jelliottp
Copy link
Author

This looks great! Thanks for working on this.

@jelliottp
Copy link
Author

jelliottp commented May 31, 2023

I have a couple other cases we've run into with standardization of spaces. I'm not sure if these still fit in this rule or not.

  1. Within IF/ELSEIF statements, we should remove extra spaces (this may apply to other statements, I haven't verified yet):

Before

IF  line_exists( some_table[ field = value ] ).
ENDIF.

After

IF line_exists( some_table[ field = value ] ).
ENDIF.
  1. Within parentheses, there is a rule to remove extra spaces for empty parentheses, but the problem exists when there are parameters as well. Perhaps these should both be handled and rename the rule to Removes multiple spaces from inside parentheses?

Before

DATA(var) = VALUE #( (  field1 = value1   ) ).

After

DATA(var) = VALUE #( ( field1 = value1 ) ).

@jmgrassau
Copy link
Member

Hi Josh,

the new options "Separate parentheses from character literals" and "Separate condensed cases …" are now available in version 1.3.0!

Regarding your last comment, the extra space in example 1. seems to be handled by the "Align logical expressions" rule already:

image

Regarding point 2, however, I think this would be worth opening a new issue, and I guess it would be for a new cleanup rule "Align components in tabular layout" (or similar). Actually this has been on my wish list for a long time already (this being a frequent case in our unit tests). Consider the following example:

  DATA(var) = VALUE some_type( ( amount =   '1.23'  name = 'short'     )
                               ( amount = 1234      name = 'longer'    )
                               ( amount =  '12.34'  name = 'very long' ) ).

I think this was carefully aligned, and the author probably wouldn't want ABAP cleaner to mess with such an alignment by removing the spaces after "amount =" or before the closing ")". Rather, they may want ABAP cleaner to create this tabular layout where possible (with lots of configuration options, of course :-), maybe even from multi-line scenarios, if the components fit into the allowed line width.

This could get a little complex, however, if not all rows have the same selection of components, if there are comments in between etc.. So – good point, but probably a larger endeavor to find a good solution!

Kind regards,
Jörg-Michael

@jelliottp
Copy link
Author

Hi @jmgrassau, I created #67 for my point 2 above.

Regarding point 1, we am not using the "Align logical expressions" rule currently, so this doesn't get handled. This is because it comes with a lot of extra alignment that isn't desired (purposely adding spaces in front of IF/WHILE/CHECK statements feels like an anti-pattern to me, but I understand if others want it).

I feel that rule is meant to handle IF statements with multiple conditions, but here I'm just trying to condense a statement with one condition. Is there some way to accommodate this situation?

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

No branches or pull requests

3 participants