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

Make application context aware of its provider #196

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

waiting-for-dev
Copy link
Member

It allows us to extract information depending on the given provider
(slice vs. application). Plus, it makes it consistent with the application
view and controller.

@jodosha
Copy link
Member

jodosha commented Dec 2, 2021

@timriley I'll leave this to your expertise 🙂

@jodosha jodosha removed their request for review December 2, 2021 11:09
Copy link
Member

@timriley timriley left a comment

Choose a reason for hiding this comment

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

Looks good, @waiting-for-dev, thank you! Would you mind making that one minor tweak before merging? Happy for you to merge right after :)

private

def define_initialize
resolve_inflector = method(:resolve_inflector)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resolve_inflector = method(:resolve_inflector)
inflector = application.inflector

Might be worth keeping this simple and only moving to a method if the complexity grows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, @timriley, done!!

It allows us to extract information depending on the given provider
(slice vs. application). Plus, it makes it consistent with the application
view and controller.
@waiting-for-dev waiting-for-dev merged commit f2ec4ae into main Dec 3, 2021
@waiting-for-dev waiting-for-dev deleted the waiting-for-dev/context_provider branch December 3, 2021 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants