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

How about ignore formatting opendal.h? #2050

Closed
wcy-fdu opened this issue Apr 20, 2023 · 9 comments · Fixed by #2056
Closed

How about ignore formatting opendal.h? #2050

wcy-fdu opened this issue Apr 20, 2023 · 9 comments · Fixed by #2056
Assignees

Comments

@wcy-fdu
Copy link
Contributor

wcy-fdu commented Apr 20, 2023

may format once bindings/c/include/opendal.h to ensure that future PRs will not change this file...

@Xuanwo
Copy link
Member

Xuanwo commented Apr 20, 2023

cargo fmt won't change opendal.h as far as I know. Maybe related to different clang-format version?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 20, 2023

Maybe we should leave opendal.h AS-IS. And don't format it by any tools...

@suyanhanx
Copy link
Member

Maybe we should leave opendal.h AS-IS. And don't format it by any tools...

An automatically generated file, that's fine to leave it AS-IS. 😅

@Xuanwo
Copy link
Member

Xuanwo commented Apr 21, 2023

cc @Ji-Xinyou, what do you think? Is it a good idea to ignore formating of opendal.h?

@xyjixyjixyji
Copy link
Contributor

cc @Ji-Xinyou, what do you think? Is it a good idea to ignore formating of opendal.h?

I tried in my machine that cargo fmt will not format the opendal.h. I not quite getting the point why we are trying to ignore the formatting of opendal.h.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 21, 2023

I tried in my machine that cargo fmt will not format the opendal.h.

It's not related to cargo fmt, the root cause is different versions (or kinds) of clang-format as we found in #2053

I not quite getting the point why we are trying to ignore the formatting of opendal.h.

Currently, we are running clang-format in build.rs to format opendal.h everytime we invoke the build of opendal-c. There are following drawbacks:

  • Developers who don't know about opendal-c also needs to install clang-format.
  • There are different kinds and versions of clang-format (6, 7, 8, 9, 10, 15) and could led to different results.
  • opendal.h is a generated file and not maintain by hands.

@xyjixyjixyji
Copy link
Contributor

I tried in my machine that cargo fmt will not format the opendal.h.

It's not related to cargo fmt, the root cause is different versions (or kinds) of clang-format as we found in #2053

I not quite getting the point why we are trying to ignore the formatting of opendal.h.

Currently, we are running clang-format in build.rs to format opendal.h everytime we invoke the build of opendal-c. There are following drawbacks:

  • Developers who don't know about opendal-c also needs to install clang-format.
  • There are different kinds and versions of clang-format (6, 7, 8, 9, 10, 15) and could led to different results.
  • opendal.h is a generated file and not maintain by hands.

I see your point. The root cause is that different version of clang-format causes different formatting results.
Considering this, I think we could ignore the formatting since it is in the end an auto-generated file.

  • If the formatting is needed by the user, they copy it to their include directory and format by themselves.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 21, 2023

Considering this, I think we could ignore the formatting since it is in the end an auto-generated file.

Great! Would you like to make changes to the Makefile and build.rs?

@Xuanwo Xuanwo changed the title cargo fmt will format opendal.h How about ignore formatting opendal.h? Apr 21, 2023
@xyjixyjixyji
Copy link
Contributor

Considering this, I think we could ignore the formatting since it is in the end an auto-generated file.

Great! Would you like to make changes to the Makefile and build.rs?

Of course, working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants