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

Remove falsy tests #12

Closed
wants to merge 1 commit into from
Closed

Remove falsy tests #12

wants to merge 1 commit into from

Conversation

blake-regalia
Copy link
Contributor

Closes #11

@blake-regalia blake-regalia requested a review from bergos January 11, 2019 21:47
@bergos
Copy link
Contributor

bergos commented Jan 14, 2019

There where also some opinions against this. As it's not clear if we keep the PR at the end, let's discuss this in the next call.

@blake-regalia
Copy link
Contributor Author

blake-regalia commented Jan 14, 2019

Hmm my point is that as it stands, the test suite is actually enforcing undefined behavior. That does not align with the interface spec. It's okay if people end up amending the spec but these test cases did not see much PR review. Can we compromise by at least commenting them out for now? Right now the suite is actually prohibiting me from publishing for this reason.

@bergos
Copy link
Contributor

bergos commented Jan 14, 2019

The compromise would be OK for me, but I have no idea how to handle that with sem versioning. As an alternative you could use a temporary git dependency in your project.

Also if you are talking about a full release and not just a beta or release candidate, I'm not sure if you do yourself a favor. The outcome of the discussion could be also that we change the spec. That would be a breaking change in your code.

@vhf
Copy link
Contributor

vhf commented Jan 15, 2019

The compromise would be OK for me, but I have no idea how to handle that with sem versioning.

If I understand correctly, the goal of this package is to implement a spec. It currently deviates from the spec by enforcing undefined behaviour.

According to semver I'd say this is a patch version (internal change that fixes incorrect behavior aka bugfix)?

@bergos
Copy link
Contributor

bergos commented Jan 15, 2019

We never discussed this, but in my opinion it's not one way from the spec to this repo. If we find differences or gaps, first we have to decide where to make the fix. That decision was not made yet.

So semver would need to cover a possible future patch version, not an actual patch version. IMHO a git branch is the right tool for this case.

@RubenVerborgh you have any feedback? It's your code from 417efb2 which I have merged.

@RubenVerborgh
Copy link
Contributor

I remember a discussion with @blake-regalia, where he was convinced: rdfjs/data-model-spec#132 (comment)

@blake-regalia
Copy link
Contributor Author

@RubenVerborgh I'm still on the fence, I'd like to hear what others have to say on the call. Let's keep this PR and the issue open for now and i'll just use the branch as @bergos suggested.

@RubenVerborgh
Copy link
Contributor

Obsoleted by proposed resolution rdfjs/data-model-spec#142.

@bergos bergos closed this Jul 24, 2019
@bergos bergos deleted the remove-falsy-tests branch June 30, 2021 21:23
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.

Remove falsy tests which don't align with interface specification
4 participants