Skip to content

Fix Mongoid Support#245

Merged
mcelicalderon merged 1 commit intographql-devise:masterfrom
jpmermoz:mongoid
Oct 20, 2022
Merged

Fix Mongoid Support#245
mcelicalderon merged 1 commit intographql-devise:masterfrom
jpmermoz:mongoid

Conversation

@jpmermoz
Copy link
Copy Markdown

These 2 small fixes allows this project to be used with Mongoid instead of ActiveRecord.

Copy link
Copy Markdown
Member

@mcelicalderon mcelicalderon left a comment

Choose a reason for hiding this comment

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

Thank you, @jpmermoz! Let a small non-blocking comment. I think the pipeline failures are unrelated, we probably just need to make more strict the gem files. I can do that in master soon if you are not able to get to that sooner.

And we should definitively add mongoid to the test matrix. It's long overdue

Comment on lines +9 to +10
column_attributes = respond_to?(:column_names) ? column_names : []
fields_attributes = respond_to?(:fields) ? fields.keys : []
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps we could simplify a bit with

Suggested change
column_attributes = respond_to?(:column_names) ? column_names : []
fields_attributes = respond_to?(:fields) ? fields.keys : []
column_attributes = try(:column_names) || []
fields_attributes = try(:fields) || []

Copy link
Copy Markdown
Author

@jpmermoz jpmermoz Oct 18, 2022

Choose a reason for hiding this comment

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

I like it.. I think the second line should be:

fields_attributes = try(:fields).try(:keys) || []

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I didn't notice the chain, so you'll need a safe operator in case try returns nil

fields_attributes = try(:fields)&.keys || []

@mcelicalderon
Copy link
Copy Markdown
Member

Thank you, @jpmermoz! This looks good. I'll just try to fix master before merging this. I did try yesterday and failed, so if this takes too long to fix I guess we could consider shipping a new version until we can get to fixing it. What do you think, @00dav00?

@mcelicalderon mcelicalderon added the bug Something isn't working label Oct 18, 2022
@00dav00
Copy link
Copy Markdown
Contributor

00dav00 commented Oct 18, 2022

I agree @mcelicalderon

@mcelicalderon
Copy link
Copy Markdown
Member

@jpmermoz I turns out the failures were just a problem with CI cache. I merged a change to refresh dependency cache. Could you please rebase your branch against master?

@jpmermoz
Copy link
Copy Markdown
Author

@mcelicalderon done! but it looks like there is still 1 failing check around rails7.0_graphql1.12.gemfile-3.0

@mcelicalderon
Copy link
Copy Markdown
Member

@jpmermoz you should have updated you fork's master with the latest from this gem before rebasing. But don't worry, it was definitively a CI problem, so merging this one. Please let us know if everything worked as expected with Mongoid after this

@mcelicalderon mcelicalderon changed the title Mongoid Fix Mongoid Support Oct 20, 2022
@mcelicalderon mcelicalderon merged commit 7873173 into graphql-devise:master Oct 20, 2022
@jpmermoz jpmermoz deleted the mongoid branch October 20, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants