Skip to content

Update to allow use of Ruby 3.0#106

Open
BiggerNoise wants to merge 5 commits intoiHiD:mainfrom
KoanHealth:main
Open

Update to allow use of Ruby 3.0#106
BiggerNoise wants to merge 5 commits intoiHiD:mainfrom
KoanHealth:main

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?

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
@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