Skip to content
This repository was archived by the owner on Jul 15, 2023. It is now read-only.

#147 implement use-simple-attribute #628

Merged
merged 6 commits into from
Nov 9, 2018
Merged

#147 implement use-simple-attribute #628

merged 6 commits into from
Nov 9, 2018

Conversation

noamyogev84
Copy link
Contributor

Fixes #147

  • implement use-simple-attribute

JoshuaKGoldberg
JoshuaKGoldberg suggested changes Nov 3, 2018
View reviewed changes
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Implementation looks generally fine. A few housekeeping comments and then we're good to go!

JoshuaKGoldberg
JoshuaKGoldberg suggested changes Nov 3, 2018
View reviewed changes
@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Nov 3, 2018
@IllusionMH IllusionMH mentioned this pull request Nov 4, 2018
Closed
@JoshuaKGoldberg JoshuaKGoldberg added the PR: Merge Target Branch Merge conflicts exist, but no other blockers to merging. label Nov 5, 2018
@noamyogev84
Copy link
Contributor Author

Hi @JoshuaKGoldberg. anything else on my side?

@JoshuaKGoldberg
Copy link

Hey @noamyogev84 - the master branch has been updated since you last sent this PR and there are some merge conflicts. They look pretty small, so you should be able to resolve them with the GitHub 'Resolve conflicts' button without too much hassle. @ me if that's not the case and I can take a look. 😊

@JoshuaKGoldberg JoshuaKGoldberg removed the PR: Waiting for Author Changes have been requested that the pull request author should address. label Nov 6, 2018
@noamyogev84
Copy link
Contributor Author

noamyogev84 commented Nov 9, 2018 •
edited
Loading

Hi @JoshuaKGoldberg .I've completed the merge. I also saw that you've removed the PR: Waiting for Author tag. Can I further assist here?

@JoshuaKGoldberg JoshuaKGoldberg removed the PR: Merge Target Branch Merge conflicts exist, but no other blockers to merging. label Nov 9, 2018
@JoshuaKGoldberg
Copy link

Thanks for the ping @noamyogev84! PR is ready to merge.

You had a couple files that had duplicated lines from the merge, so I went ahead and removed them for you. Then I noticed there were a couple of small nitpicks I'd missed in the review (rule should have a plural name per its description; update the description in README.md as well) and didn't want to be annoying and ask you to do them after the review, so I hope you don't mind me having submitted them myself 😊.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 4b91110 into microsoft:master Nov 9, 2018
@noamyogev84
Copy link
Contributor Author

@JoshuaKGoldberg Thanks. I appreciate your attitude and help!

@IllusionMH IllusionMH added this to the 6.0.0 milestone Feb 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@noamyogev84 @JoshuaKGoldberg @IllusionMH