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

[ISSUE #4521] A poor naming. #4523

Closed
wants to merge 9 commits into from
Closed

[ISSUE #4521] A poor naming. #4523

wants to merge 9 commits into from

Conversation

Asymtode712
Copy link

Fixes #4521

Motivation

Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.

Modifications

Describe the modifications you've done.

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@Asymtode712
Copy link
Author

@pandaapo Please review my PR and suggest changes if there exists any.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to the Apache EventMesh community!!
This is your first PR in our project. We're very excited to have you onboard contributing. Your contributions are greatly appreciated!

Please make sure that the changes are covered by tests.
We will be here shortly.
Let us know if you need any help!

Want to get closer to the community?

WeChat Assistant WeChat Public Account Slack
Join Slack Chat

Mailing Lists:

Name Description Subscribe Unsubscribe Archive
Users User support and questions mailing list Subscribe Unsubscribe Mail Archives
Development Development related discussions Subscribe Unsubscribe Mail Archives
Commits All commits to repositories Subscribe Unsubscribe Mail Archives
Issues Issues or PRs comments and reviews Subscribe Unsubscribe Mail Archives

@Asymtode712 Asymtode712 changed the title Updated the 'Errors' class name and its occurances [ISSUE #4521]Updated the 'Errors' class name and its occurances Oct 30, 2023
@Asymtode712 Asymtode712 changed the title [ISSUE #4521]Updated the 'Errors' class name and its occurances [ISSUE #4521] Updated the 'Errors' class name and its occurances Oct 30, 2023
@Pil0tXia Pil0tXia mentioned this pull request Oct 30, 2023
3 tasks
@pandaapo
Copy link
Member

Duplicate with #4501.

There is no description in issue #4501 that indicates that Errors will be modified. So, this cannot be considered duplicate.


issue #4501中没有任何描述能体现出会修改Errors。所以,这不能说重复。

https://github.com/Pil0tXia/eventmesh/blob/pil0txia_feat_4501/eventmesh-admin/src/main/java/org/apache/eventmesh/admin/enums/Status.java

This is a branch of your own repository.


这是你自己仓库的一个分支。

@Asymtode712
Copy link
Author

Asymtode712 commented Oct 30, 2023

As my old commit was being tagged Duplicate by @Pil0tXia. I have changed the class name to 'Test'.
My new commit is all free from any duplication from any previous PRs.
@pandaapo Kindly take a look at it

Copy link
Member

@pandaapo pandaapo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your modifications were a bit bad. And you haven't checked any other references to this class.

@Asymtode712
Copy link
Author

Asymtode712 commented Oct 30, 2023

@pandaapo i used the class name 'Status' in my last commit with was tagged as Duplicate by @Pil0tXia. If you allow me i can rollback to the previous version of the commit and can change it back to 'Status'.

@Asymtode712
Copy link
Author

Am i allowed to do it @pandaapo ?

@pandaapo
Copy link
Member

@pandaapo i used the class name 'Status' in my last commit with was tagged as Duplicate by @Pil0tXia. If you allow me i can rollback to the previous version of the commit and can change it back to 'Status'.

You can rollback or change to another appropriate name. In addition, you need to check other references to this class.

@Pil0tXia
Copy link
Member

Pil0tXia commented Oct 30, 2023

The usage of this exception enumeration class has already undergone some changes in my development branch. Renaming it based on the existing master branch would make it difficult to merge my branch. If you want this issue to be fixed as soon as possible, I can submit a portion of the code first. If you're not in a hurry, you can see these changes in the PR of #4501. @pandaapo


这个异常枚举类的用法在我的开发分支中已经发生了一些改变,根据现有的master分支进行重命名,会导致我的分支合入困难。如果您希望这个问题被尽快修复,我可以先提交一部分代码。如果您不着急的话,您会在 #4501 的PR中见到这些更改。@pandaapo

@Asymtode712
Copy link
Author

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

@Pil0tXia
Copy link
Member

@Asymtode712 Hi, welcome. I'm sorry, but the changes you're making conflict significantly with my development work. You might want to consider working on a different good first issue. Additionally, editing code in this way using GitHub's online editor is not recommended. It's better to use an IDE that supports the Java language for code refactoring.

@Asymtode712
Copy link
Author

@Pil0tXia I have discovered all the occurences using VS Code only. I have just replaced them using Github's editor.

@Pil0tXia
Copy link
Member

Pil0tXia commented Oct 30, 2023

@Asymtode712 I'm sorry that this PR doesn't correspond with EventMeshAdmin's development plan and it conflicts a lot. I'm the author of eventmesh-admin and this module is under active development. If you can try another good first issue, I'd be happy to review it for you. You may feel free to close this pr. Again, my apologies.

@Asymtode712
Copy link
Author

Asymtode712 commented Oct 30, 2023

Apologies @Pil0tXia, but i would like to hear @pandaapo's take on this

@pandaapo
Copy link
Member

If you want this issue to be fixed as soon as possible, I can submit a portion of the code first.

The issue is not urgent, but I hope you can submit a portion of the code about it first. As Asymtode712 has already submitted PR, I will review this PR when I have time. If there is no better PR and there are no problems with this PR, there is no reason not to approve it. Please understand. Thank you.


该issue不是很急,但是我希望你能先提交关于该issue的代码。由于Asymtode712 已经提交,有空的时候我将审核该PR。如果没有更好的PR,同时该PR没有任何问题,我就没有理由不通过该PR。请理解,谢谢。

@pandaapo
Copy link
Member

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

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

@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #4523 (bf71efd) into master (a091017) will not change coverage.
The diff coverage is n/a.

❗ Current head bf71efd differs from pull request most recent head fdeb233. Consider uploading reports for the commit fdeb233 to get more accurate results

@@            Coverage Diff            @@
##             master    #4523   +/-   ##
=========================================
  Coverage     15.91%   15.91%           
  Complexity     1545     1545           
=========================================
  Files           730      730           
  Lines         28896    28896           
  Branches       2745     2745           
=========================================
  Hits           4600     4600           
  Misses        23816    23816           
  Partials        480      480           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pandaapo pandaapo changed the title [ISSUE #4521] Updated the 'Errors' class name and its occurances [ISSUE #4521] A poor naming. Oct 31, 2023
@pandaapo
Copy link
Member

pandaapo commented Oct 31, 2023

PR #4524 is better, so I will approve it. Thank you all the same for your work and sorry about to close this. You can try other 'good first issues' that have not been claimed or processed by anyone yet.

@xwm1992 xwm1992 closed this Oct 31, 2023
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.

[Enhancement] A poor naming.
4 participants