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

Add rules for function and parameter to have the closing parenthesis on its own line #1103

Closed
JHertz5 opened this issue Jan 17, 2024 · 9 comments · Fixed by #1108
Closed
Assignees

Comments

@JHertz5
Copy link
Contributor

JHertz5 commented Jan 17, 2024

Is your feature request related to a problem? Please describe.
procedure_006 and function_300 can both enforce indentation of the closing parenthesis if it is on its own line. However, I can't find any rule to enforce that closing parenthesis being on its own line.

Describe the solution you'd like
I would like to be able to enforce the closing parenthesis of a function/procedure to be on its own line. For example, I'd like the code:

package my_package is

  function my_function (
    i_arg1 : integer;
    i_arg2 : boolean) return integer;

  procedure my_procedure (
    i_arg1 : integer;
    i_arg2 : boolean);

end package my_package;

package body my_package is

  function my_function (
    i_arg1 : integer;
    i_arg2 : boolean) return integer is
  begin

    return i_arg1;

  end function;

  procedure my_procedure (
    i_arg1 : integer;
    i_arg2 : string) is
  begin

    report i_arg2;

  end procedure;

end package body my_package;

to be formatted to

package my_package is

  function my_function (
    i_arg1 : integer;
    i_arg2 : boolean
  ) return integer;

  procedure my_procedure (
    i_arg1 : integer;
    i_arg2 : boolean
  );

end package my_package;

package body my_package is

  function my_function (
    i_arg1 : integer;
    i_arg2 : boolean
  ) return integer is
  begin

    return i_arg1;

  end function;

  procedure my_procedure (
    i_arg1 : integer;
    i_arg2 : string
  ) is
  begin

    report i_arg2;

  end procedure;

end package body my_package;
@JHertz5
Copy link
Contributor Author

JHertz5 commented Jan 17, 2024

I will try to raise a PR for this. At the moment I am planning a "simple solution" of a rule each for functions and procedures that enforces the new line. I suppose it would be disabled by default to maintain the current default behaviour.

However, this does not allow enforcement of the inverse, i.e. no newline before the closing parenthesis. In order to have that configurability, I think it might be better to just roll the structure rules for functions and procedure together into a procedure_call_003 style structure rule. I would probably need some guidance before trying that, so unless I hear otherwise, I'll just stick with the simple solution.


Update: I've had another look and I have managed to put together a more general rule for subprogram specification structure, similar to procedure_call_003, so I think that this will be a nice, tidy solution. Just working on cleanup, testing and documentation now.

@JHertz5
Copy link
Contributor Author

JHertz5 commented Jan 20, 2024

I have raised PR #1108 for this issue. It is not yet ready for review, as I am still working on the unit tests. It is now ready for review and any feedback is welcome. There are quite a lot of changes here, so @jeremiah-c-leary, please let me know if anything is not to your liking (or make changes to the branch as you see fit).

@JHertz5
Copy link
Contributor Author

JHertz5 commented Jan 21, 2024

In addition to the closing parenthesis, the new rules also enforce the newlines around the opening parenthesis, interface list semcolons and interface elements. There are not indentation rules for all of these yet, but that PR is already quite large (and much larger than I originally intended), so I think that the missing indentation rules should be implemented as a separate issue/when someone requests it.

@jeremiah-c-leary
Copy link
Owner

Morning @JHertz5 ,

so I think that the missing indentation rules should be implemented as a separate issue/when someone requests it.

I agree. The current indent rules will cover the most likely structural styles.

The pull request looks really good. I appreciate the documentation and the tests you added to cover the new rules.

I just had several minor comments in the PR. When we address those I will merge this to master.

How did you find the process of adding these rules? Anything I can do to make it easier or less confusing?

Regards,

--Jeremy

@JHertz5
Copy link
Contributor Author

JHertz5 commented Jan 24, 2024

Hi @jeremiah-c-leary

Thanks very much!

I just had several minor comments in the PR. When we address those I will merge this to master.

Perfect, thanks. I'll address any comments as soon as I get a chance, should be on the weekend at the latest. I can't see any comments on the PR at the moment. Have they not been submitted yet?

How did you find the process of adding these rules? Anything I can do to make it easier or less confusing?

I found it to be straightforward now that I've built some familiarity with your codebase. My strategy was to find existing rules that were similar to what I wanted to implement, and use those as a template for the new rule, using bin/vsg_parser to identify the tokens within the snippet of VHDL that I used to test the new rule. So, for myself, I don't think there is anything that caused confusion or difficulty.

If you'd like my opinion on the experience for a new contributor, I have a couple of thoughts. Things like using bin/vsg_parser to identify tokens and using bin/vsg_rule_doc_gen to update the documentation are extremely useful for a contributor but are not obvious or easy to discover without your guidance. I've seen some extremely helpful comments that you've very kindly written to individuals (including myself), describing how to create a new rule, set up tests, etc. but this information isn't easy to find. I think it would be useful to have something like a broad step-by-step guide to producing a rule, or just a "useful tips for contributors" page?

@jeremiah-c-leary
Copy link
Owner

Morning @JHertz5 ,

Perfect, thanks. I'll address any comments as sson as I get a chance, should be on the weekend at the latest. I can't see any comments on the PR at the moment. Have they not been submitted yet?

The comments show up in the conversion tab of the PR. If you scroll towards the bottom you will see the three comments I made.

Image

Thank you for the feedback on the contributing experience, it is obviously a blind spot for me.

I really should document the process of adding a new rule. That would bring out all the tools available. I could also describe the patterns you see in creating tests, etc...

I will create a ticket to improve what little documentation I have.

Thanks,

--Jeremy

@JHertz5
Copy link
Contributor Author

JHertz5 commented Jan 25, 2024

Hi @jeremiah-c-leary

The comments show up in the conversion tab of the PR. If you scroll towards the bottom you will see the three comments I made.

I can see in the screenshot that you posted that the comment has the "Pending" label. This means that the comments have been added as part of a review which has not been submitted yet, and no one other than yourself can see them until you submit the review.
This is what I see on the PR currently:


image

image

Thank you for the feedback on the contributing experience, it is obviously a blind spot for me.

No problem at all!

@jeremiah-c-leary
Copy link
Owner

Evening @JHertz5 ,

I can see in the screenshot that you posted that the comment has the "Pending" label. This means that the comments have been added as part of a review which has not been submitted yet

I just submitted the review, so I hope you can see the comments now.

--Jeremy

@JHertz5
Copy link
Contributor Author

JHertz5 commented Jan 26, 2024

Thanks very much @jeremiah-c-leary! I can see those comments now. I have actioned/responded to each and requested your re-review.

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

Successfully merging a pull request may close this issue.

2 participants