Skip to content

Simpler provider set handling through reflection #18

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

Merged
merged 1 commit into from
Aug 16, 2012

Conversation

tjdett
Copy link
Contributor

@tjdett tjdett commented Aug 16, 2012

Sets can be now be configured by having a :set attribute, or by having proper set models that relate back to the provider model.

Overriding Provider.find should no longer be necessary, which avoids the possibility of messing up resumption token behaviour.

Sets can be configured by having a :set attribute, or by having proper set objects. Overriding Provider.find should no longer be necessary.
@travisbot
Copy link

This pull request passes (merged 8c2efbf into 04ad0da).

@tjdett
Copy link
Contributor Author

tjdett commented Aug 16, 2012

@cbeer While I could merge this myself, I'd appreciate any feedback you might have on this pull-request.

It's a reasonably large change to the way ActiveRecordWrapper works, though it should produce the same behaviour for existing code.

@cbeer
Copy link
Member

cbeer commented Aug 16, 2012

looks good to me (although I've never used the ARWrapper, so it's really your call). Does this change require a minor version bump, do you think?

@tjdett
Copy link
Contributor Author

tjdett commented Aug 16, 2012

It does add new backwards-compatible behaviour, so assuming we're using semantic versioning it does require a minor version bump.

I'd give it a few days to see if something of similar size comes along, after which I think it would be worth bumping to 0.3.0.

tjdett added a commit that referenced this pull request Aug 16, 2012
Simpler provider set handling through reflection
@tjdett tjdett merged commit 538dc89 into code4lib:master Aug 16, 2012
@cbeer
Copy link
Member

cbeer commented Aug 16, 2012

+1

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