Skip to content

fix(c): remove redundant newline and fix $0 position in main snippents #451

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

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

REmerald
Copy link
Contributor

Kind of reverts 7a57697 and #443

The main point of not putting $0 on newly indented line is because it's way easier for a user to press enter than pressing esc then dd if inserting if with empty {} was intended. See #237 (comment).

Copy link
Collaborator

@OkelleyDevelopment OkelleyDevelopment left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but perhaps we can bring in some others so we're all on the same page since it's reverting some changes.

Will wait to merge, so others can view them as well.

cc: @ilan-schemoul @rafamadriz

@OkelleyDevelopment
Copy link
Collaborator

Assuming there isn't any push back between now and the end of the week, I'll merge this PR then.

@surmish
Copy link
Contributor

surmish commented Jun 3, 2024

The mainn code snippet seems fine as is? It is a fairly common snippet and not having a newline adds unnecessary keystroke. The newline before return EXIT_SUCCESS seems like it should be retained too...

@OkelleyDevelopment
Copy link
Collaborator

Hi @REmerald, see the comment above. It might be the case we can decline this PR for now 🤔

@REmerald
Copy link
Contributor Author

@OkelleyDevelopment Other snippets with braces (if, switch, for, while, functions, struct) has been following this style for 1.5 years and everybody has been ok with it. Nobody changed them through pull requests, even @ilan-schemoul changed only these 2 and didn't touch other mentioned above.

So,

It is a fairly common snippet and not having a newline adds unnecessary keystroke.

is not a valid argument, also for reasons explained in pr description.

The newline before return EXIT_SUCCESS seems like it should be retained too...

Any logic behind this? Plus, many people use format-on-save and clang-format will remove it anyways if you don't have any code between the brace and the newline.

@ilan-schemoul
Copy link
Contributor

Kind of reverts 7a57697 and #443

The main point of not putting $0 on newly indented line is because it's way easier for a user to press enter than pressing esc then dd if inserting if with empty {} was intended. See #237 (comment).

I don't see the need for empty main but sure we can revert to be consistent

@REmerald
Copy link
Contributor Author

REmerald commented Jul 5, 2024

I don't see the need for empty main

It's for organizational purposes, the user may decide to write some functions before main, but still leave it empty, e.g. not to forget to write it afterwards.

but sure we can revert to be consistent

@OkelleyDevelopment surmish put the thumbs-up emoji, so everybody seems to agree. Can it be merged now?

@ilan-schemoul
Copy link
Contributor

It's for organizational purposes, the user may decide to write some functions before main, but still leave it empty, e.g. not to forget to write it afterwards.

I really don't agree we should do everything according to a fringe case.
But again if it's a matter of consistency and habits we can revert.

@OkelleyDevelopment
Copy link
Collaborator

Thanks everyone for your input!

I will merge this PR for the consistency as the repo stands.

Going forward, perhaps we should apply a higher level of scrutiny regarding the structure of snippets being added/modified to avoid similar discrepancies from reoccurring 🤔

@OkelleyDevelopment OkelleyDevelopment merged commit 45a1b96 into rafamadriz:main Jul 8, 2024
@REmerald REmerald deleted the patch branch July 22, 2024 22:06
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