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

MAINT: unify code style #1236

Open
2 tasks
marscher opened this issue Nov 18, 2024 · 8 comments
Open
2 tasks

MAINT: unify code style #1236

marscher opened this issue Nov 18, 2024 · 8 comments
Labels
enhancement Improvement in capability planned for future release

Comments

@marscher
Copy link
Member

marscher commented Nov 18, 2024

Currently we do not have or use a coding style. For instance:

  • Some files are indented by tabs, other by spaces.
  • We do not fully comply with pep8 (e.g. camel casing in Python).
  • Cpp files are not uniformly formatted.
  • Java files also different styles.

To mitigate some of the problems, I'd suggest to use formatting tools like Ruff, clang-format etc with rule-sets stored in the repository. That way everybody can use them. We could also enforce them later with git commit hooks.

I think the project would benefit in the long term by doing that, because it lowers the burden of reading into the code for new developers.

EDIT:

setup CI to use

  • Ruff
  • clang-tidy
@marscher marscher added the enhancement Improvement in capability planned for future release label Nov 18, 2024
@Thrameos
Copy link
Contributor

Thrameos commented Nov 18, 2024

I am a bit warry of changing our casing in Python to be more Python like. We once have start_jvm and startJVM IRC. We are talking to Java and thus presenting a camel case language. I don't have any problem with making variable names follow the Python conventions, but our public interface is what it is and we shouldn't make changes to it.

(Darn autocorrect changed shouldnt to should)

@marscher
Copy link
Member Author

you're right, we should not change the API and the camel case argument is strong.

@astrelsky
Copy link
Contributor

There is also the need to be careful with python formatting tools since some Java imports need to come after the JVM is started. I'm not sure how much of the jpype codebase that would effect though.

I'm a big fan of the clang tools. while I haven't used clang format all that much, I like clang-tidy. It's nice to have a modern linter for C/C++. It complains about everything (which is a good thing), and is about as configurable as linters for other modern languages.

For the camel case Python API, you could always just create aliases. This way it will always "feel right" whether you're coming from Python or Java.

It looks like NetBeans has support for the eclipse formatter, as do most Java IDEs I think. It's probably the best bet for Java.
https://plugins.netbeans.apache.org/catalogue/?id=15

@pelson
Copy link
Contributor

pelson commented Nov 19, 2024

since some Java imports need to come after the JVM is started.

My worst nightmare 😄 (#933)

For the camel case Python API, you could always just create aliases

The thing is that this would also need to alias the Java methods too... then when you implement an interface there is an ambiguity which form you should implement, and even a risk that there are two different implementations.

I think it is an idea worth discussing (if it hasn't already been discussed elsewhere) - I would be interested to explore the idea, but probably best as its own topic.


To mitigate some of the problems, I'd suggest to use formatting tools like Ruff

👍 on ruff. I would suggest we use pre-commit for iterative improvements rather than doing it in a single big-bang formatting change.

@marscher
Copy link
Member Author

@ALL lets please forget about the Python camel casing. We can also set it up as a rule we're explicitly breaking (in Ruff).

@astrelsky Which commands/linting configuration would you recommend using for C++ utilizing clang-tidy? I can then set it up for continuous integration.

@astrelsky
Copy link
Contributor

@ALL lets please forget about the Python camel casing. We can also set it up as a rule we're explicitly breaking (in Ruff).

@astrelsky Which commands/linting configuration would you recommend using for C++ utilizing clang-tidy? I can then set it up for continuous integration.

I'll pull the commands from my ide logs tonight. I figure that's more helpful than telling you I don't know 😂

@Thrameos
Copy link
Contributor

Thrameos commented Nov 19, 2024

We have automatic formatting for Python on checkin. We just need a working beautifier for C and Java.

@astrelsky
Copy link
Contributor

@ALL lets please forget about the Python camel casing. We can also set it up as a rule we're explicitly breaking (in Ruff).

@astrelsky Which commands/linting configuration would you recommend using for C++ utilizing clang-tidy? I can then set it up for continuous integration.

The attached file was generated with the command below. I'm assuming those CheckOptions are the defaults but I'm not sure why it would explicitly add them if they are. The checks I've disabled are specifically because there isn't much we can do about it and/or the warning was just annoying. The most annoying one you're going to get, that was intentionally left enabled, is the unused parameter one. Since we need to conform to the correct native Java method and python function signatures, we have a lot of unused parameters. They can be silenced by simply casting the parameter to void, ie (void)self. Unlike CodeQL, these are usually helpful instead of complete 💩 without a way to silence them. I can see them in my fork and it's amusingly bad.

clang-tidy -p compile_commands.json C:\Users\astre\Documents\jpype\native\common\jp_array.cpp --checks="bugprone-*,clang-diagnostic-*,clang-analyzer-*,performance-*,misc-*,concurrency-*,-performance-no-int-to-ptr,-bugprone-easily-swappable-parameters,-misc-use-anonymous-namespace,-bugprone-empty-catch,-bugprone-reserved-identifier,-misc-non-private-member-variables-in-classes" --dump-config

There is a convenient list of the check groups here. I just manually disable some once I get fed up and think a specific check isn't helpful. https://clang.llvm.org/extra/clang-tidy/#using-clang-tidy

Leave ALL alone, they didn't do anything wrong!

githubisdumb.clang-tidy.txt

it should obviously be .clang-tidy, GitHub is just dumb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement in capability planned for future release
Projects
None yet
Development

No branches or pull requests

4 participants