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

Let Administrate figure out the associations #44

Merged
merged 5 commits into from
Aug 20, 2021

Conversation

pablobm
Copy link
Contributor

@pablobm pablobm commented Nov 26, 2020

Following thoughtbot/administrate#1633, this PR would bring this plugin to take advantage of Administrate's own association detection, doing away with the :class_name option.

Note that this breaks compatibility with versions of Administrate prior to 0.15.

@pablobm
Copy link
Contributor Author

pablobm commented Nov 26, 2020

Uh, CircleCI doesn't appear to be using Administrate master? Hm, not a big issue for now I guess.

@nickcharlton
Copy link
Owner

Is the CI problem because of Appraisal? A thought here: we could point an Appraisal set to master, and just let the rest fail until it's updated upstream.

As for breaking changes: I think we're okay there, we'll just need to make a note in the CHANGELOG when we release.

@pablobm pablobm force-pushed the automatic-associations branch 2 times, most recently from 41226cb to 161a584 Compare December 3, 2020 14:06
@pablobm
Copy link
Contributor Author

pablobm commented Dec 3, 2020

Oh, I forgot about Appraisal. You are right, it works after removing the appraisals for old versions. So either this is merged without back compatibility, or I spend a bit more time to make it work with older versions.

By the way, we already have some code to do back compatibility. If we are merging as is, we probably should also remove that:

<%
current = Gem::Version.new(Administrate::VERSION)
v0_10 = Gem::Version.new("0.10")
%>
<% if current <= v0_10 %>
<%= render "fields/nested_has_many/show_old", **local_assigns %>
<% else %>
<%= render "fields/nested_has_many/show_current", **local_assigns %>
<% end %>

Perhaps we can wait for the next version of Administrate and I'll do two PRs: one to remove compatibility with older versions, and another would be this PR.

@pablobm pablobm mentioned this pull request Mar 25, 2021
@pablobm
Copy link
Contributor Author

pablobm commented Mar 25, 2021

As mentioned I have opened PR #46, which will remove the compatibility with Administrate prior to v0.10

@pablobm pablobm force-pushed the automatic-associations branch from 161a584 to 8cae5a5 Compare April 22, 2021 09:28
@pablobm pablobm marked this pull request as ready for review April 22, 2021 09:54
@pablobm
Copy link
Contributor Author

pablobm commented Apr 22, 2021

Took this PR out of draft mode. I think this can be considered for merge now.

@nickcharlton nickcharlton merged commit 9ec7ca6 into nickcharlton:master Aug 20, 2021
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.

2 participants