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 for constant variables. #3046

Closed
jzdayz opened this issue Jun 12, 2020 · 12 comments · Fixed by #5773
Closed

Enhancement for constant variables. #3046

jzdayz opened this issue Jun 12, 2020 · 12 comments · Fixed by #5773

Comments

@jzdayz
Copy link
Contributor

jzdayz commented Jun 12, 2020

Issue Description

Type: code review

Describe what happened (or what feature you want)

I noticed that there are some magic variables in applicationUtils and PropertyUtil, can it be written in one place,it's so messy. i think all config magic variables should be one place

image
image

Describe what you expected to happen

If it passes, I can do this

How to reproduce it (as minimally and precisely as possible)

Tell us your environment

Anything else we need to know?

@jzdayz
Copy link
Contributor Author

jzdayz commented Jun 12, 2020

image
so messy

@jzdayz
Copy link
Contributor Author

jzdayz commented Jun 12, 2020

  • plan1
public interface Constants {
    interface Raft{

    }

    interface Nacos{
        String FUNCTION_MODE_PROPERTY_NAME = "nacos.functionMode";
    }

    interface Web{
        String ACCEPT_CHARSET = "Accept-Charset";
        String SERVER_PORT = "server.port";
    }
}
  • plan2
public class Constants {
    public static class Raft{

    }

    public static class Nacos{
        public static String FUNCTION_MODE_PROPERTY_NAME = "nacos.functionMode";
    }

    public static class Web{
        public static String ACCEPT_CHARSET = "Accept-Charset";
        public static String SERVER_PORT = "server.port";
    }
}

@KomachiSion
Copy link
Collaborator

Looks great, but I can't get the difference between the two plan, I think plan two is more used.

If you want to do this, please pay attention to the scope of these constants.

@jzdayz
Copy link
Contributor Author

jzdayz commented Jun 12, 2020

Looks great, but I can't get the difference between the two plan, I think plan two is more used.

If you want to do this, please pay attention to the scope of these constants.

ok i will do it

@KomachiSion KomachiSion changed the title about variable problems Enhancement for constant variables. Jun 19, 2020
@KomachiSion
Copy link
Collaborator

@jzdayz Any progress?

@jzdayz
Copy link
Contributor Author

jzdayz commented Jul 20, 2020

@jzdayz Any progress?

Sorry, in my idea, I want to put all the constants in common module, but I found that there are constant classes in different modules, so I think that if the code is modified, the range of changes will be larger. I think the code in the future It can be unified, but I dare not move the current code

@KomachiSion
Copy link
Collaborator

@jzdayz you can split this issue as multiple sub task. each module each task. And refactor for each module.
After all modules has finished, then to think about merge constants and unified them.

@KomachiSion
Copy link
Collaborator

For example:

  • refactor constants in naming module
  • refactor constants in config module
    ....
  • merge and unified constants for modules.

@jzdayz
Copy link
Contributor Author

jzdayz commented Jul 27, 2020

For example:

  • refactor constants in naming module
  • refactor constants in config module
    ....
  • merge and unified constants for modules.

OK,i got this , let me think , modify one by one

@KomachiSion
Copy link
Collaborator

@jzdayz There is no process for a long time, I unassign this issue for you first. If you want to solve it, please relay again with new process.

@KomachiSion KomachiSion modified the milestones: 1.4.0, 1.4.1 Sep 22, 2020
@KomachiSion KomachiSion modified the milestones: 1.4.1, 1.4.2 Jan 13, 2021
@KomachiSion KomachiSion modified the milestones: 1.4.2, 1.4.3 Apr 29, 2021
@li-xiao-shuang
Copy link
Collaborator

@i will solve it@

@KomachiSion
Copy link
Collaborator

This PR has no finished.
but it can be do with #5726 .

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

Successfully merging a pull request may close this issue.

5 participants