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

[Enhancement] A poor naming. #4521

Closed
1 of 3 tasks
pandaapo opened this issue Oct 30, 2023 · 18 comments · Fixed by #4524
Closed
1 of 3 tasks

[Enhancement] A poor naming. #4521

pandaapo opened this issue Oct 30, 2023 · 18 comments · Fixed by #4524
Assignees
Labels
good first issue Issues for first-time contributors

Comments

@pandaapo
Copy link
Member

pandaapo commented Oct 30, 2023

Search before asking

  • I had searched in the issues and found no similar issues.

Enhancement Request

public enum Errors {
SUCCESS(HttpStatus.OK, Types.SUCCESS, "Operation success."),

A class that defines multiple response states, including SUCCESS state, but the class name is Errors. Bad smell should be avoided in our codes. Because it will reduce code's readability and maintainability, and also confuse developers.

Describe the solution you'd like

Change the class name to an appropriate name.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@pandaapo pandaapo added the good first issue Issues for first-time contributors label Oct 30, 2023
@Asymtode712
Copy link

Hey @pandaapo i want to work on it. Please assign me

@pandaapo
Copy link
Member Author

Welcome! @Asymtode712

@Asymtode712
Copy link

Asymtode712 commented Oct 30, 2023

Thanks @pandaapo for assigning it, before making all the changes just wanted to ask you, would 'Faults' be an appropriate replacement for 'Errors'

@Pil0tXia
Copy link
Member

@pandaapo
Copy link
Member Author

would 'Faults' be an appropriate replacement for 'Errors'

No, I don't think it is appropriate either.

@Pil0tXia
Copy link
Member

Pil0tXia commented Oct 30, 2023

@Pil0tXia Yes, I saw this reply #4498 (comment). But I haven't seen any related issues or PR in EventMesh community. In addition, I will not visit your repository specifically, nor will I go to your repository to find a branch to check.

@pandaapo Since you've pointed out this issue, I agree with your perspective, and you've also seen my response, so you will definitely find the changes to this class in my next PR. Please rest assured. 😏

@pandaapo
Copy link
Member Author

@Pil0tXia Yes, I saw this reply #4498 (comment) 9 days ago.
But I haven't seen any related issues or PR in EventMesh community.
In addition, I will not visit your repository specifically, nor will I go to your repository to find a branch to check.


我在9天前看到该回复#4498 (comment)
但我并没有在EventMesh社区看到任何相关的issue和PR。
另外,我不会专门去看你的仓库,更不会去你的仓库中找一个分支出来看。

@pandaapo
Copy link
Member Author

Since you've pointed out this issue, I agree with your perspective, and you've also seen my response, so you will definitely find the changes to this class in my next PR.

@Pil0tXia
After you reply #4498 (comment), your next PR was #4518.


在你回复了 以后,你的下一个PR是#4518

@Asymtode712
Copy link

@pandaapo Please review my PR and suggest if there are some changes to be done.

@Pil0tXia
Copy link
Member

@pandaapo #4518 has nothing to do with EventMeshAdmin and the changes were made to #4501 in 10.21 immediately after you mentioned naming problem.

#4518 is a quick fix for users. When #4517 was spotted, the naming change has already done.

image

@pandaapo
Copy link
Member Author

@pandaapo Please review my PR and suggest if there are some changes to be done.

I have assigned this issue to the author of this code as well now. I will merge a better PR among you if the author accept the issue. Are you willing to accept the issue? @Pil0tXia

@Pil0tXia
Copy link
Member

@pandaapo #4523 didn't make much change, and it will obviously conflict with #4501. If you find my response reasonable, you can allocate your valuable time to other tasks, and just leave this matter to me.

@pandaapo
Copy link
Member Author

pandaapo commented Oct 30, 2023

the changes were made to #4501 in 10.21 immediately after you mentioned naming problem.

the naming change has already done.

@Pil0tXia
I have already said that I will not specifically go to your repository to find a branch to see your daily work, no one will. Who else knows about the modifications you made on your own branch besides you?


我已经说了我不会专门去你的仓库中找一个分支来看你平时的修改,任何人都不会。你在你自己的分支上做的修改,除了你还有谁会知道?

@Pil0tXia
Copy link
Member

Pil0tXia commented Oct 30, 2023

I had already provided an affirmative response before I submitted the code to my own branch. Since you've also seen that reply, if you're concerned about my development progress or whether I've forgotten about this matter, you can contact me directly below my response or mention me in a new issue, without the need to open a good first issue.

Of course, a new issue is also not a problem. I explained the situation to you right away, indicating that the issue has already been resolved, and it is planned to be submitted along with another PR. You can rest assured.

It's OK to leave this issue to me.


在我向我自己的分支提交代码之前,就已经给您肯定的回复了。既然您也看到了那条回复,如果您关心我的开发进度,以及我有没有忘掉这件事情,您可以直接在我的回复下面联系我,或者在新issue里@我,不用开放一个good first issue的。

当然,一个issue也没什么,我也第一时间跟您解释清楚情况了,说明了这个问题已经被解决了,以及它随另一个PR提交的计划。您大可放心。

这个issue交给我就行,有劳了!

@Asymtode712
Copy link

@pandaapo I have made all the changes. Please review my PR

@pandaapo
Copy link
Member Author

@Pil0tXia
I focus more on the project itself. As for the issue, it's not a very urgent demand, I never thought about who to urge or who must complete it. After a long time, thinking of this code and not seeing any related issues and PR, I think I can raise an issue.

Now I have also assigned the issue to you and explained that I will merge the better one. But it seems that you are more willing to argue than submit a PR of some relevant code first. I believe that as the author of this code, you can quickly and resolve this issue better. I am quite busy with work today, so I don't have time to participate in community affairs before I finish work. Please understand. Thank you.

If I haven't seen your PR after work and Asymtode712's PR completes the issue well before your PR submission, I will approve it first.


我更多的是只关注项目本身。就该issue而言,并不是很急的需求,没想过要催促谁,也没认为必须谁来完成。经过很长时间,想起这段代码,同时也没看到任何相关issue和PR,我觉得可以提个issue。

现在我已经也将该issue分配给你,并说明会择优合并。但你似乎更愿意辩论,而不先提交一部分相关代码的PR。我相信你作为该代码的作者能很快更好地解决该issue。今天工作比较忙,下班之前我没有时间参与社区事务。请理解,谢谢。

如果下班后没有看到你的PR,同时Asymtode712的PR能很好地完成该issue,我会先review通过。

@pandaapo
Copy link
Member Author

@pandaapo I have made all the changes. Please review my PR

I will take some time to review the relevant PRs after work today.

@Pil0tXia
Copy link
Member

@pandaapo No problem. 👌 #4524

xwm1992 pushed a commit that referenced this issue Oct 31, 2023
* rename common package to constant

* rename Errors enum to Status and Message sub-class to StatusMessage

* Exact 'Types' to first level of classification 'Category'

* remove 'error' in Status javadoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues for first-time contributors
Projects
None yet
3 participants