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

Add support for multiple readers #12

Closed
wants to merge 6 commits into from

Conversation

janmyszkier
Copy link
Contributor

No description provided.

Copy link
Owner

@mlesniew mlesniew left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, great work.

I added a few comments, would you mind making corrections before I merge it?

src/elicznik/elicznik.py Outdated Show resolved Hide resolved
src/elicznik/elicznik.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@janmyszkier
Copy link
Contributor Author

@mlesniew sorry, I somehow missed those comments (I pretty much ignore issues not assigned to me directly). I'll take a look at the CR and make the adjustments today.

@janmyszkier janmyszkier changed the title Ad support for multiple readers Add support for multiple readers Apr 9, 2024
@janmyszkier
Copy link
Contributor Author

@mlesniew not sure if you have seen the updated, so just ping-ing you here.

@mlesniew
Copy link
Owner

Hey, sorry I haven't look into this for so long.

Thanks for applying the requested changes. There were a few additional changes I wanted to introduce before merging, but since they were simple and this PR has been waiting for so long, I went ahead and applied them myself.

I have:

  • squashed your work into just two commits and reworded the description
  • removed nargs="?" from the --site parameter specification (surprisingly setting nargs this way doesn't make it work as intuitively expected)
  • made some cosmetic changes to the README and the help message.

The changes are now merged into master. You're still the author of the two commits. I will soon release a new version with these changes and push it to PyPI.

Thanks for your contribution!

@mlesniew mlesniew closed this May 19, 2024
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.

2 participants