Skip to content

Feature/changesets #44

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 28, 2016
Merged

Feature/changesets #44

merged 5 commits into from
Jan 28, 2016

Conversation

@no-reply
Copy link
Member

@no-reply no-reply commented Jan 3, 2016

This makes changes relevant for #250.

Tom Johnson added 4 commits January 24, 2016 10:08
Test Mutable#apply_changeset
9f27199 
Recent changes expand changesets to be applicable to any
mutable. Responsibility for applying the changeset atomically lives
within this method in mutable, which can be called directly or through
`Changeset#apply`.
Rework transaction specs for 2.0
dee4755
Soften RDF::Transaction buffering requirements
986d8a3 
Not every `Transaction` needs to keep an up-to-date `Changeset` in
buffer. This change loosens the requirements. some of the tests are
removed to the `rdf` test suite in `spec/transaction_spec.rb`.
Expect Transaction spec users to give a Repository
ac0515d 
`RDF::Transaction` subclasses will generally depend upon a specific
`RDF::Repository`. It's best to expect the user of the shared examples
to provide the repository class. This follows the pattern seen in the
`RDF::Enumerable` examples and others.
@no-reply
Copy link
Member Author

no-reply commented Jan 25, 2016

This depends on ruby-rdf/rdf#250. One of the two will need to be merged before the other for CI to pass.

@no-reply no-reply mentioned this pull request Jan 25, 2016
Merged
@gkellogg
Copy link
Member

gkellogg commented Jan 25, 2016

I think that removing Enumerable from Queryable is something that should happen to 2.0, if at all, as it's a breaking change. If we leave it to 2.1, we'd be breaking semantic versioning.

Most uses of Queryable methods (e.g., #first) probably use a pattern, when they don't then some enumerator is necessary. Note that first isn't very useful unless the solutions are ordered; it's really more like sample.

Queryable#enum_for leverages Enumerable#enum_for for enumerating solutions.

OTOH, for people to be able to use a Transaction consistently, it probably has to be Enumerable. We should probably go with your second solution and implement #each. If this proves to be a performance issue, we can address at a later date.

@gkellogg
Copy link
Member

gkellogg commented Jan 25, 2016

Otherwise, I think this is probably good to go.

@no-reply
Copy link
Member Author

no-reply commented Jan 26, 2016

OTOH, for people to be able to use a Transaction consistently, it probably has to be Enumerable. We should probably go with your second solution and implement #each. If this proves to be a performance issue, we can address at a later date.

👍. If Transactions become Enumerable, is there any reason they shouldn't also be Readable? I suppose I could try adding the mixin and running the tests; but if you can warn me off a yak shave, that would be great.

no-reply pushed a commit that referenced this pull request Jan 28, 2016
Merge pull request #44 from ruby-rdf/feature/changesets
4425aa8 
Feature/changesets
@no-reply no-reply merged commit 4425aa8 into develop Jan 28, 2016
@no-reply no-reply deleted the feature/changesets branch January 28, 2016 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@no-reply @gkellogg