Skip to content

Sorted constants in alphabetically order #625

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

Closed
wants to merge 4 commits into from
Closed

Sorted constants in alphabetically order #625

wants to merge 4 commits into from

Conversation

sagorika1996
Copy link

@sagorika1996 sagorika1996 commented Aug 5, 2017

Addressed to #622

@sagorika1996 sagorika1996 changed the title Sorted constants in alphabetically order #622 Sorted constants in alphabetically order Aug 5, 2017
@mystamps-bot
Copy link

mystamps-bot commented Aug 5, 2017

2 Errors
🚫 maven-checkstyle-plugin error in src/main/java/ru/mystamps/web/Url.java#L121:
Line has trailing spaces
🚫 danger check: pull request contains merge commits! Please, rebase your branch to get rid of them: git rebase master master and git push --force-with-lease
1 Warning
⚠️ danger check: branch master does not comply with our best practices. Branch name should use the following scheme: ghXXX_meaningful-name where XXX is an issue number. Next time, please, use this scheme :)
1 Message
📖 maven-checkstyle-plugin reported about 1 error. Please, fix it. See also: https://github.com/php-coder/mystamps/wiki/checkstyle

Generated by 🚫 Danger

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.727% when pulling 790f004 on sagorika1996:master into eb57501 on php-coder:master.

@sagorika1996 sagorika1996 changed the title #622 Sorted constants in alphabetically order Sorted constants in alphabetically order Aug 5, 2017
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

First of all, thank you! I left a couple of comments but overall it looks good to me.

Also I see that you use your name as an author for the commit. Generally I advise to use a real name (but it's not required of course). So, consider updating the author value in the commit (git commit user.name "John Dow" && git commit --am --reset-author).

BTW how did you find this project?

@@ -120,29 +120,29 @@ private Url() {
public static Map<String, String> asMap(boolean serveContentFromSingleHost) {
// There is not all urls but only those which used on views
Map<String, String> map = new HashMap<>();
map.put("PUBLIC_URL", PUBLIC_URL);
Copy link
Owner

Choose a reason for hiding this comment

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

Could you also add a comment here?

@@ -76,12 +76,12 @@
if (userDetails.isAdmin()) {
authorities.add(Authority.ADD_COMMENTS_TO_SERIES);
Copy link
Owner

Choose a reason for hiding this comment

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

Could you also add a comment here and above?

@sagorika1996
Copy link
Author

Thanks for the comments! This is my first time trying out GitHub, so I was searching for some trivial bug fixes to get started with and found this.

Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

See comments from a bot:

  • trailing space should be removed
  • there should be only single commit (not 3)

@php-coder
Copy link
Owner

@sagorika1996 Hi, do you have plans to fix my comments or you aren't interested in anymore?

@php-coder
Copy link
Owner

@sagorika1996 Hi, I don't want to reject your PR but I also can't merge it because it contains 4 commits (instead of 1) and there are couple trivial comments that should be addressed. Please, try to fix them and I'd be happy to accept your contribution!

@php-coder
Copy link
Owner

I'm sorry I can't accept this PR because my comments weren't fixed. If it was a single commit, I would probably merge it. But it has 4 commits for such a trivial change and also I'll need to fix minor issues by myself after that. So I feel that it doesn't worth it.

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

Successfully merging this pull request may close these issues.

4 participants