-
Notifications
You must be signed in to change notification settings - Fork 34
Sort constants in alphabetical order #771
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
Conversation
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
=========================================
Coverage 79.38% 79.38%
Complexity 448 448
=========================================
Files 28 28
Lines 1232 1232
Branches 173 173
=========================================
Hits 978 978
Misses 240 240
Partials 14 14 Continue to review full report at Codecov.
|
Hi,
Thank you for this PR! I’m on holidays now and can’t review or merge it
from my phone. I’ll look at it in 3 days.
…--
Slava Semushin
|
@@ -128,33 +128,34 @@ private Url() { | |||
boolean serveContentFromSingleHost = !production; | |||
|
|||
// Not all URLs are listed here but only those that are being used on views | |||
// Constant sorted in ascending order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you modify it to "Constants sorted in an ascending order." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good but I'd like to modify the comments just a bit. Could you update all the comments? Thanks!
Yeah, you are right, I just fixed them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also sort constants and add a comment here:
mystamps/src/main/java/ru/mystamps/web/Url.java
Lines 160 to 171 in 7d3776d
map.put("BOOTSTRAP_CSS", BOOTSTRAP_CSS); | |
map.put("BOOTSTRAP_JS", BOOTSTRAP_JS); | |
map.put("JQUERY_JS", JQUERY_JS); | |
map.put("SELECTIZE_CSS", SELECTIZE_CSS); | |
map.put("SELECTIZE_JS", SELECTIZE_JS); | |
map.put("GET_IMAGE_PAGE", GET_IMAGE_PAGE); | |
map.put("GET_IMAGE_PREVIEW_PAGE", GET_IMAGE_PREVIEW_PAGE); | |
map.put("FAVICON_ICO", FAVICON_ICO); | |
map.put("MAIN_CSS", MAIN_CSS); | |
map.put("CATALOG_UTILS_JS", CATALOG_UTILS_JS); | |
map.put("SERIES_ADD_JS", SERIES_ADD_JS); | |
map.put("COLLECTION_INFO_JS", COLLECTION_INFO_JS); |
and there:
mystamps/src/main/java/ru/mystamps/web/Url.java
Lines 173 to 180 in 7d3776d
// Use a separate domain for our own resources | |
map.put("GET_IMAGE_PAGE", STATIC_RESOURCES_URL + GET_IMAGE_PAGE); | |
map.put("GET_IMAGE_PREVIEW_PAGE", STATIC_RESOURCES_URL + GET_IMAGE_PREVIEW_PAGE); | |
map.put("FAVICON_ICO", STATIC_RESOURCES_URL + FAVICON_ICO); | |
map.put("MAIN_CSS", STATIC_RESOURCES_URL + MAIN_CSS); | |
map.put("CATALOG_UTILS_JS", STATIC_RESOURCES_URL + CATALOG_UTILS_JS); | |
map.put("SERIES_ADD_JS", STATIC_RESOURCES_URL + SERIES_ADD_JS); | |
map.put("COLLECTION_INFO_JS", STATIC_RESOURCES_URL + COLLECTION_INFO_JS); |
Thanks in advance!
Done, check it out |
Merged in 78e9e94 commit. Thanks! |
@GiBg1aN Let me know if you need more tasks like this one. |
I found this trivial abandoned PR, so did it from scratch. (Addressed to #622)