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

Update to allow use of Ruby 3.0 #106

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

BiggerNoise
Copy link
Contributor

Addresses #105

I wasn't able to get the 3.0 syntax for the Publisher::publish() to work with ruby 2.6 although it does work with 2.7. I made my condition < 3 to ensure we didn't poke current users.

I had to include an XML parser for 3.0, happy to pull in a different one if you'd like.

I think there's a bigger P/R to change to named arguments throughout (instead of the options hash), but that's going to give you breaking changes. I'm happy to do the work, but let's open an issue so we can discuss logistics before I start throwing stuff over the wall.

Copy link
Owner

@iHiD iHiD 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 this!

@dougal Could you take a look too please?

propono.gemspec Outdated Show resolved Hide resolved
lib/propono/services/publisher.rb Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
test/services/publisher_test.rb Show resolved Hide resolved
Copy link
Collaborator

@dougal dougal left a comment

Choose a reason for hiding this comment

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

Looks fine. Actually simpler than I expected when I read #105.

@BiggerNoise BiggerNoise force-pushed the main branch 3 times, most recently from 7acd889 to 792640a Compare January 20, 2022 15:59
@BiggerNoise
Copy link
Contributor Author

It's been ages since I made a P/R, so forgive any etiquette missteps.

I'm going to resolve the remaining conversations b/c I think I have addressed the issues with the two commits I made today. Please let me know if there are any further changes you'd like to see.

@BiggerNoise BiggerNoise requested a review from iHiD January 20, 2022 18:32
.github/workflows/tests.yml Outdated Show resolved Hide resolved
propono.gemspec Show resolved Hide resolved
@iHiD
Copy link
Owner

iHiD commented Jan 21, 2022

@BiggerNoise It's all great, thanks. One request on the git sha and one question about nokogiri, but then I'll get it merged and cut.

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.

3 participants