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

Remove extension for Sinatra #116

Merged
merged 1 commit into from
Jul 17, 2022
Merged

Remove extension for Sinatra #116

merged 1 commit into from
Jul 17, 2022

Conversation

epergo
Copy link
Member

@epergo epergo commented Apr 16, 2021

Contributes to #114

After the Sinatra extension has been extracted to its own gem/repository now we can remove the extension code from Mustermann itself.

@zzak
Copy link
Member

zzak commented Apr 16, 2021

I'm wondering what will be the experience if this code is simply removed, would it be better if we provided the same interface but raise an exception to the user telling them to add the extension gem?

@epergo
Copy link
Member Author

epergo commented Apr 17, 2021

I'm wondering what will be the experience if this code is simply removed, would it be better if we provided the same interface but raise an exception to the user telling them to add the extension gem?

Right! The joy of deleting code blinded me and I didn't think about that 😄 I can restore the extend_object method but instead of loading the removed code we can raise an exception with an explanatory message, would that work?

@namusyaka
Copy link
Member

@epergo Thanks for your contribution.

I think you can simply put an exceptional message in mustermann/lib/mustermann/extension.rb, and then you can restore only extend_object without register Extension.
At least you should consider a case that requires mustermann/extension

@epergo
Copy link
Member Author

epergo commented Apr 18, 2021

@epergo Thanks for your contribution.

I think you can simply put an exceptional message in mustermann/lib/mustermann/extension.rb, and then you can restore only extend_object without register Extension.
At least you should consider a case that requires mustermann/extension

Updated in dcbf027 WDYT?

@@ -120,7 +120,6 @@ def self.normalized_type(type)
def self.extend_object(object)
Copy link
Member

Choose a reason for hiding this comment

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

Is this code called from anywhere? It seems we want to remove the require in this method 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, Sinatra's register method calls extend, which calls extend_object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct it is used here

Copy link
Member Author

Choose a reason for hiding this comment

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

@zzak Do you want me to do any change? Maybe I missed the point.

Copy link
Member

Choose a reason for hiding this comment

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

@epergo Sorry, I was just wondering out loud nothing actionable -- but I think it'd be good to test this out a bit before we release anything

@olleolleolle
Copy link
Member

@epergo Hi 👋 ! This is awesome, and shows a way forward for mustermann + 3.0. Does it still work? (GitHub Actions' logs are purged, swept away.)

@epergo
Copy link
Member Author

epergo commented Sep 13, 2021

@epergo Hi 👋 ! This is awesome, and shows a way forward for mustermann + 3.0. Does it still work? (GitHub Actions' logs are purged, swept away.)

I've updated the PR with master, ruby 3.0 job still fails because of 1 test and the coverage report

@jkowens jkowens closed this Jul 16, 2022
@jkowens jkowens reopened this Jul 16, 2022
@jkowens jkowens merged commit eeed94c into sinatra:master Jul 17, 2022
@epergo epergo deleted the ep/remove-sinatra-extension branch July 17, 2022 01:38
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.

6 participants