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

Set a max of 1 consecutive blank line #7

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EduPonz
Copy link

@EduPonz EduPonz commented Feb 22, 2023

No description provided.

Comment on lines +1984 to 1987
nl_max = 2 # unsigned number

# The maximum number of consecutive newlines in a function.
nl_max_blank_in_func = 0 # unsigned number
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nl_max = 2 # unsigned number
# The maximum number of consecutive newlines in a function.
nl_max_blank_in_func = 0 # unsigned number
nl_max = 3 # unsigned number
# The maximum number of consecutive newlines in a function.
nl_max_blank_in_func = 2 # unsigned number

Copy link
Author

Choose a reason for hiding this comment

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

This would leave room to have either 1 or 2 blanks before things like function or class definitions, which IMO should be fixed to either 1 (I just used 1 because our code base does more or less that) or 2 (as for instance pep8 enforces for python). What I would not do is to leave it up to the contributor, I'd rather have some uniformity

Copy link

Choose a reason for hiding this comment

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

Fair concern. We could use this set of configurations for that then:

# The number of newlines before a function prototype.
nl_before_func_body_proto       = 0        # unsigned number

# The number of newlines before a multi-line function definition.
nl_before_func_body_def         = 0        # unsigned number

# The number of newlines before a class constructor/destructor prototype.
nl_before_func_class_proto      = 0        # unsigned number

# The number of newlines before a class constructor/destructor definition.
nl_before_func_class_def        = 0        # unsigned number

Copy link
Member

Choose a reason for hiding this comment

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

This was more a suggestion than a request (that's why I only commented).

I agree with your opinion, but we should take into account our whole code base, since this is the file for all the code in the eProsima repositories.

I haven't checked all of them, but I think you are right that we are almost always using a single blank line everywhere.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, definitely! That's why I'd like as many opinions as possible here

@EduPonz EduPonz marked this pull request as draft February 22, 2023 14:48
@EduPonz
Copy link
Author

EduPonz commented Feb 22, 2023

I've changed this PR to draft until we reach an agreement. It is missing:

  • Update README with changes

@JLBuenoLopez
Copy link
Contributor

JLBuenoLopez commented Feb 22, 2023

I've changed this PR to draft until we reach an agreement.

This is going to be merged the same day #6 is merged... And that one has been waiting for a couple of years to reach an agreement 🤣

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.

4 participants