Skip to content

Comments

Use collections.Counter instead of a dict - Closes #7868#7874

Merged
Zac-HD merged 3 commits intopytest-dev:masterfrom
tanvimehta:master
Oct 8, 2020
Merged

Use collections.Counter instead of a dict - Closes #7868#7874
Zac-HD merged 3 commits intopytest-dev:masterfrom
tanvimehta:master

Conversation

@tanvimehta
Copy link
Contributor

@tanvimehta tanvimehta commented Oct 8, 2020

Use collections.Counter instead of a dict in terminal.py Closes #7868

Tanvi Mehta and others added 2 commits October 7, 2020 21:51
Use `collections.Counter` instead of a `dict` in `terminal.py` Issue pytest-dev#7868
@tanvimehta
Copy link
Contributor Author

@Zac-HD Would appreciate it if you could help review this. Thanks!

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

This looks great to me - thanks @tanvimehta, and congrats on your first Pytest PR 🎉

I'd also encourage you to add your name to the list of contributors in https://github.com/pytest-dev/pytest/blob/master/AUTHORS, and edit your PR description to use the magic keywords to automatically close the linked issue (e.g. Closes #7868 instead of closes issue #7868 - putting "issue" in the middle disables it).

Then we'll be ready to merge 🤩

@tanvimehta tanvimehta changed the title Use collections.Counter instead of a dict - Closes Issue #7868 Use collections.Counter instead of a dict - Closes #7868 Oct 8, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Oct 8, 2020

Apparently keywords only work in the body, not the title - you should see a little line under the keyword (and tooltip) when it's working, including in the preview.

(FYI this is not at all a blocker, I'd merge it anyway, but the point here is to level you up as a OSS contributor 💖)

@tanvimehta
Copy link
Contributor Author

Thanks a lot @Zac-HD. I added the magic word to the body(confirmed that it works as expected) and added my name to the AUTHORS file. I assumed this change would be considered trivial so I wasn't sure about adding my name to the authors' list.

@Zac-HD Zac-HD merged commit dbd082a into pytest-dev:master Oct 8, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Oct 8, 2020

I assumed this change would be considered trivial so I wasn't sure about adding my name to the authors' list.

Fair enough! From one perspective it is trivial, in that it probably took me longer to write it up and review your PR than it would have to just write the patch myself. But IMO that's the wrong perspective - we did it this way because mentoring and community matter for us (pytest and the wider Python OSS community), and you absolutely are a contributor. So don't be shy about adding yourself to the list on other projects - the length of that list is a point of pride for us 😁


One bit of advice for next time: I just noticed that you made the PR from your master branch - it's better to use a PR-specific branch, which lets you work on more than one PR or feature at a time. Later on you can also do fancy tricks like rebasing on the latest upstream version to fix any conflicts.

And finally, ✨CONGRATULATIONS✨ on the PR 🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use collections.Counter instead of a dict in terminal.py

2 participants