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

[DO NOT MERGE] Clang format styles #267

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

LecrisUT
Copy link
Collaborator

@pboettch I just want to get your opinion on different clang-format styles. Check json-schema.hpp in the commits for the output differences

Option A: 4bff849

This is a minimal adaptation of the current format, just changing the namespace indentation to make it more readable.

Option B: e192837

Complete overhaul based off of clion defaults. This is what I usually use, and it mostly resolves some nitpicks I have with bracing position, indicator for reference/pointer etc.

Do you have any preference or different opinions on these?

LecrisUT added 17 commits May 10, 2023 10:37
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
- Enforce pre-commits
- Move conan to CD workflow

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Temporarily disabled until setup internally

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
- Added JSON_VALIDATOR_SHARED_LIBS to properly handle shared-library
- Bumped minimum cmake to 3.11 to use no-source add_library
- Bumped minimum cmake to 3.14 to properly support FetchContent (FetchContent_MakeAvailable)
- Converted Hunter package manager to FetchContent (It is plenty mature these days)
- Added namespace to exported target
- Made the cmake file compatible with FetchContent

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Not an ideal approach, but required in order for the exported target to have appropriate linkage.
Maybe this can be designed to become a PRIVATE link library, but then how does it ensure the target is installed?

Signed-off-by: Cristian Le <[email protected]>
This will be moved to packaging integration tests

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@pboettch
Copy link
Owner

No and no.

Option 1: I don't like indentation for namespaces. And I like indentation for pre-processor-directives (if they are complex)

Option 2: Unacceptable for me. There are so many things wrong in my eyes. Especially the & and * position in arguments and variable declarations. The & and * belongs to the variable not to the type:

Type* var1, var2;

What's the type of var2? How do you write it as a pointer? Type* var1, * var2 ?

But thanks for the proposal, thanks to clang-format, I hope it didn't take you too much time.

I'm however open for real improvement. E.g. pre-processor indents, why not scrap them, if it's doing too much non-sense. For the moment, I think it is OK.

@LecrisUT
Copy link
Collaborator Author

No problem, option B are my personal nitpicks where I believe you should define values and pointers separately anyway. But yeah those personal preferences so no problem having different opinions.

But let's discuss option A a bit more. The indentation of the preprocessor is kept the same, it mostly changes namespace indentation. In that file for example it was hard to navigate to where nlohmann::json_schema namespace began. That is not an issue with nlohmann/json because there only one namespace is used (excluding the macro defined one), but in this file both parent and child namespaces are used which creates the navigation problem.

Alternatively though if only one namespace is used (with aliases on the parent namespace if needed) that would also solve the readability issue.

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