Skip to content

Conversation

lightalloy
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor

Description

  • fixed new rubocop issues
  • generated a new .rubocop_todo.yml

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jun 21, 2019
- 'app/models/article.rb'

# Offense count: 2101
# Offense count: 2415
Copy link
Contributor

Choose a reason for hiding this comment

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

ahhaha this keeps increasing :D

if params[:config] == "all_comments" && current_user_is_author?
@notification_subscription.notifiable.update(receive_notifications: true)
end
@notification_subscription.notifiable.update(receive_notifications: true) if params[:config] == "all_comments" && current_user_is_author?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a mega fan of the giant ifs at the end, i think the code was more readable before. I'm thinking we should not enforce this, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I haven't checked the autocorrected stuff properly.

'Content-Type': 'application/json',
},
body: JSON.stringify({poll_vote: { poll_option_id: optionId } }),
body: JSON.stringify({poll_skip: {poll_id: pollId }}),
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be part of this PR? It seems it's changing behavior here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird, have you tried to rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, "up-to-date".

Copy link
Contributor

Choose a reason for hiding this comment

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

It happened to me once or twice, what I did was to create a new branch, cherry pick the commits from the old one in order and either force push or create a new PR.

I still don't know why :(

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW the second commit contains the change so cherry picking it won't work e2f6837

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if you look at line 81 red and green - they are identical, and the line 96 red and green - they are identical as well (apart from the indentation).
But the git diff looks confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's confusing.

@lightalloy lightalloy requested a review from rhymes June 21, 2019 10:13
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jun 21, 2019
@benhalpern benhalpern merged commit f79b3cb into forem:master Jun 21, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants