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

Reformat Client/API: use file-scope namespace and primary constructor #729

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

xlegalles
Copy link
Contributor

closes #

@xlegalles
Copy link
Contributor Author

@Zelldon you take no risk at merging these PR: all was done automatically by JetBrains Resharper and is 100% accurate (it's not AI)

@ChrisKujawa
Copy link
Collaborator

ChrisKujawa commented Oct 23, 2024

Sorry @xlegalles I'm super busy with #camunda/camunda project right now.

I wanted to check how git blame looks now whether it breaks history but looks ok.

I have some questions:

  • You mentioned there is inconsistency between usage of namespaces. Is that really true? I feel like, and remember that that I used most of the time namespace with brackets 🤔
    • Not saying that it is better, just curious because of the inconsistency.
    • Might be better to just move the other which are not using brackets, as this might be less of a change? 🤔
  • We have style cop, can we make use of it somehow? Break the build if it is not consistent for example.

@xlegalles
Copy link
Contributor Author

xlegalles commented Oct 24, 2024

@Zelldon I understand you are busy but I am too and I have currently a time window which allows me to work on this ZeebeClient but it's not going to last long. The goal is to implement the streaming ability but before I want to do this clean up. I use JetBrains Resharper as a lot of C# developers do (including MS guys) and it is not incompatible with Style Cop: the fact is that even style cop rules are not properly applied and my IDE pops warnings everywhere. It does not break the build but is still very annoying.

All to say that I just try to remove warnings and also "refresh" a bit this source code: file-scope namespaces (C# 8) is better because it reduces the identation and this is the same idea with primary constructors which allows a more compact code. Why would be the reasons to not apply them? Same comment about nullable references: we have to apply it to this entire source code because you will see how many checks are currently missing which could end into ArgumentNullException.

I am sure you don't hesitate yourself to change Java source code the same way when it's for good reasons.

@ChrisKujawa
Copy link
Collaborator

ChrisKujawa commented Oct 24, 2024

It does not break the build but is still very annoying.

That for sure.

All to say that I just try to remove warnings and also "refresh" a bit this source code: file-scope namespaces (C# 8)

Thanks for sharing I was not aware of this change before :)

Why would be the reasons to not apply them?

You might misunderstood me. It was more about of whether it would be quicker to align the files to namespaces with brackets if you want to do it because of a matter of consistency (that was your argument in the different PR).

In general, I'm open to reducing the boilerplate and unnecessary indention.

I am sure you don't hesitate yourself to change Java source code the same way when it's for good reasons.

Not sure what you mean with this.


As you split the PRs already (thanks for this). I would expect that each PR would target a specific module/project or namespace/folder.

Here we seem to target the API based on the diff https://github.com/camunda-community-hub/zeebe-client-csharp/pull/729/files, but we somehow missed several files? Is this intended?

-> Can you also reformat them then? So everything under Client/API

@ChrisKujawa ChrisKujawa changed the title style: use file-scope namespace and primary constructor Reformat Client/API: use file-scope namespace and primary constructor Oct 24, 2024
@ChrisKujawa
Copy link
Collaborator

I understand you are busy but I am too and I currently have a time window

BTW It is obviously also a matter of priority. This project has little to none prio with my work life, it is more a hobby/slack project to me.

I would be happy to promote you to a maintainer if you're interested.

@xlegalles
Copy link
Contributor Author

Well, to be honnest, my company is interested by this project as we use it in production. So I don't work on it on my spare time, which is more comfortable than you.
I accept to be the maintainer if you stay around anyway ;)

@xlegalles
Copy link
Contributor Author

BTW, I have split the initial PR based on the number of files, not by folder/namespace because some have more files than others

@ChrisKujawa ChrisKujawa merged commit a5e3407 into camunda-community-hub:main Oct 24, 2024
5 checks passed
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