Skip to content

644-simplify a step by opening a series #712

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 5 commits into from
Closed

644-simplify a step by opening a series #712

wants to merge 5 commits into from

Conversation

olithin
Copy link

@olithin olithin commented Nov 29, 2017

Addressed to #643, #644, #647, #648

@olithin olithin requested a review from php-coder as a code owner November 29, 2017 05:46
@olithin olithin changed the title just open a page with a series (/series/1) 644-simplify a step by opening a series Nov 29, 2017
@php-coder
Copy link
Owner

php-coder commented Nov 29, 2017

Отлично! Спасибо!

Хорошо, что ты сделала это отдельными коммитами, но это все равно порождает некоторые трудности с ревью. В следующий раз, лучше создай 4 реквеста с коммитом в каждом -- так я быстрее приму правки. Например, сейчас к одному из коммитов у меня есть пожелания и пока ты его будешь править, остальные коммиты будут ждать. Вдобавок, тебе самой будет сложнее внести правки в 3 коммит с конца, так как тут нужно довольно хорошо знать git.

@php-coder
Copy link
Owner

Так, первое что нужно сделать, это обновить твой мастер и потом бранч:

  1. git checkout master
  2. git fetch upstream
  3. git merge upstream/master
  4. git rebase master task644
  5. git push --force-with-lease --all

@php-coder
Copy link
Owner

Так, один из твоих коммитов я уже смерджил. Спасибо!

@olithin olithin self-assigned this Dec 18, 2017
@olithin
Copy link
Author

olithin commented Dec 18, 2017

I coudn't commit simple rename of folder.That's why I did tasks together in one request

@mystamps-bot
Copy link

2 Errors
🚫 maven-checkstyle-plugin error in src/test/java/ru/mystamps/web/it/cucumber/step/StepDefinitions.java#L25:
‘org.openqa.selenium.By’ should be separated from previous imports
🚫 maven-checkstyle-plugin error in src/test/java/ru/mystamps/web/it/cucumber/step/StepDefinitions.java#L28:
‘ru.mystamps.web.Url’ should be separated from previous imports
2 Warnings
⚠️ danger check: pull request contains 5 commits while most of the cases it should have only one. If it’s not a special case you should squash the commits into single one.
But be careful because it can destroy all your changes!
⚠️ danger check: branch task644 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 2 errors. Please, fix them. See also: https://github.com/php-coder/mystamps/wiki/checkstyle

Generated by 🚫 Danger

@php-coder
Copy link
Owner

I coudn't commit simple rename of folder.That's why I did tasks together in one request

Жаль не спросила. Ну либо можно было бы сделать эту задачу последней, после всех этих правок.

Мерджить пока ничего не могу -- и бот ругается и коммитов много :( Как только исправишь импорты, я еще раз посмотрю и подумаю как тебе разобраться с коммитами, чтобы их можно было мерджить.

Я уже говорил, что если бы ты сделала один коммит и один PR, то все эти 4 задачи уже давно было бы закрыты? :)

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.

3 participants