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

fix style of last argument of argument lists #341

Closed
wants to merge 1 commit into from

Conversation

andlaus
Copy link
Collaborator

@andlaus andlaus commented Sep 10, 2024

instead of sometimes using a comma for the last item of a list and sometimes not, we now always close lists of items immediately. This does not come with any functional changes, but it makes the coding style a bit more consistent and IMO the code is slightly more readable with this style.

Andreas Lauser <[email protected]>, on behalf of MBition GmbH.
Provider Information

instead of sometimes using a comma for the last item of a list and
sometimes not, we now always close the list in the same line. This
does not come with any functional changes, but it makes the coding
style a bit more consistent and IMO the code is slightly more readable
with this style.

Signed-off-by: Andreas Lauser <[email protected]>
Signed-off-by: Gerrit Ecke <[email protected]>
@andlaus
Copy link
Collaborator Author

andlaus commented Sep 10, 2024

@kayoub5: this is motivated by a comment which you made a while ago :)

@@ -118,11 +118,9 @@ def find_named_object(item_list: List[T], name: str) -> T:
byte_position=0,
coded_value=FLIC_FLAC_SID,
bit_position=None,
sdgs=[],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep the comma always, it makes reading diffs easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

automating the implementation of this is pretty difficult, because for trivial stuff like f(x) we probably don't want the trailing comma. (and I really don't want to do this fully manually.) That said, I'm also happy to close this PR unmerged and keep the current state of the art...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is if the list or function arguments are multi-line, a trailing comma helps better with diffs

A trailing comma is not needed for a one line list

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. I agree that it makes sense, but my point was that writing a script for "semi-automatic" refactoring for this style is pretty hard... (for the current PR I could basically get away with a bit of regex replacement magic, but I don't see a way to get around a "bracket+string" parser if I have to count the number of arguments, etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

under these constraints, shall we close or merge?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just close

@andlaus andlaus closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants