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

Rethinking property deduplication #169

Open
rossabaker opened this issue Jun 21, 2020 · 2 comments
Open

Rethinking property deduplication #169

rossabaker opened this issue Jun 21, 2020 · 2 comments

Comments

@rossabaker
Copy link
Member

When a rule set duplicates ids from a parent ruleset, the rule set's properties are silently dropped in favor of the parent's. This happened twice in Cats (typelevel/cats#3493), both times unwittingly and unintentionally.

The scaladoc requires:

The only requirement here is that ''inside one kind'', the identifier of a property is unique, since duplicates are eliminated.

We could offer an idCollisions: Set[String] on the RuleSet, and leave it up to the test integrations to fail when non-empty, but this would be a breaking change. Alternatively, we could deduplicate with an obnoxious suffix instead of silently dropping duplicate properties, which should be safe unless people are relying on this deduplication. I don't think they should be for anything but optimization, and Discipline's most prominent client, Cats, was bitten by the current behavior.

@rossabaker rossabaker changed the title Silent deduplication Rethinking property deduplication Jun 21, 2020
@larsrh
Copy link
Contributor

larsrh commented Jun 21, 2020

Could discipline throw an error instead?

@rossabaker
Copy link
Member Author

I thought about deprecating all and creating an allProps that throws, or perhaps returns Either. It would avoid any semantic change to discipline-core, but all the test integrations would need to adopt it.

If we just make all throw, scala-steward would do most of the rest of the work downstream.

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

No branches or pull requests

2 participants