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

Readme amends #115

Merged
merged 4 commits into from
Jun 21, 2017
Merged

Readme amends #115

merged 4 commits into from
Jun 21, 2017

Conversation

Cleop
Copy link
Member

@Cleop Cleop commented Jun 21, 2017

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #115 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   98.08%   98.08%           
=======================================
  Files          22       22           
  Lines         157      157           
=======================================
  Hits          154      154           
  Misses          3        3
Impacted Files Coverage Δ
web/controllers/rules/issue/time_estimation.ex 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f91b9d...1d1371f. Read the comment docs.

@Cleop Cleop requested review from nelsonic and SimonLab June 21, 2017 10:58
@Cleop Cleop changed the title WIP- Readme amends Readme amends Jun 21, 2017
README.md Outdated

| Category | Rule | dwylbot comments | Labels changed |
|---------------|------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------|
| Pull Requests | "awaiting-review" and a merge conflict, #64 | :warning: @username, the pull request has a **merge conflict**., Please resolve the conflict and reassign when ready :+1:, Thanks! | remove "awaiting-review" and replace with "merge-conflicts". @username assigned. |
Copy link
Member

Choose a reason for hiding this comment

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

The issue number ie: #64 are not linked to the issues, we might need to precise the full url?


We've devised a set of rules and actions to respond to these problems, they look like this:

| Category | Rule | dwylbot comments | Labels changed |
Copy link
Member

Choose a reason for hiding this comment

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

The format looks nice 👍
I think we should also add a column "trigger" which explain what trigger dwylbot, ex: "on PR open", "on adding the in-progress label",...

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I had tried to include in the 'rule' column, would you like me to rephrase them?

I think I called it 'rule' rather than say 'trigger' because in the readme I'm thinking about it from a user over developer perspective... the user wants to know what our gitflow rules are to decide whether they want to use dwylbot because it's useful/relevant to their own gitflow.

Copy link
Member

Choose a reason for hiding this comment

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

I think rules and triggers are slightly different (but sometime they can be the same).
For me the rule is all the conditions the issue/pr must satisfied. The trigger is what is going to start the verification of the rule. Different type of triggers can also start the same rule.

For example for the rule that check if an issue has the description will only be triggered when the issue is created and dwylbot shouldn't run the rule on other triggers, ie we don't want the rule when we close the issue, or when someone change a label,...
Let me know what you think, maybe I'm going to much in details for the readme but I think it can help users to reproduce a error if they need to test it.

Copy link
Member Author

@Cleop Cleop Jun 22, 2017

Choose a reason for hiding this comment

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

Right, that's interesting. So in theory, say I created an issue with a blank description and dwylbot warned me against this. I then continue working on the issue and adding labels to it/ closing it etc. - I should only receive the warning from dwylbot that I should give my issue a description once, when I first created it? I won't get a reminder every time I do another action on the issue?

I suppose from a technical perspective that would mean you'd have to be doing more evaluations on each post event because any of the rules might apply at that time if the user had failed to act upon the first warning.

Copy link
Member

@SimonLab SimonLab left a comment

Choose a reason for hiding this comment

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

LGTM, I've added a couple of comments but no blockers:

  • add trigger action
  • add link to the issue

@SimonLab SimonLab removed their assignment Jun 21, 2017
@Cleop Cleop assigned Cleop and unassigned nelsonic Jun 21, 2017
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@Cleop great improvements. 👍

@nelsonic nelsonic merged commit 117e3d6 into master Jun 21, 2017
@nelsonic nelsonic deleted the readme-amends branch June 21, 2017 17:01
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