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 double-keyed sets and maps #174

Closed
wants to merge 1 commit into from

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented May 2, 2019

Before this PR

Rust doesn't support double-keyed maps and sets, and Go has significant difficulties implementing the required behavior as well.

On the Rust side in particular, we can't simply ignore those tests since the generated code from the Conjure definitions doesn't even compile.

After this PR

The offending types and tests cases are removed.

Fixes #173

@sfackler sfackler requested a review from a team as a code owner May 2, 2019 17:33
Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little hesitant to see these test cases go away since they're the only ones exercising the notion of equality defined here for doubles. Can we workaround this in conjure-rust to avoid having to make the change here?

@sfackler
Copy link
Member Author

sfackler commented May 2, 2019

The workaround would probably be to either add code generation functionality to skip specific objects and endpoints, or to edit the test IR file and filter the offending bits out there.

@ferozco
Copy link
Contributor

ferozco commented May 2, 2019

The later sounds simpler and more palatable to me. Would you be ok going that route instead?

@uschi2000
Copy link

what does the conjure spec say about this?

@sfackler
Copy link
Member Author

sfackler commented May 3, 2019

The conjure spec says they need to behave like they do in Java, which is a problem for other languages: palantir/conjure#178

@uschi2000
Copy link

uschi2000 commented May 3, 2019

ok, thanks @sfackler . Then I think we need to change the tests and the spec at the same time, and also update the parsers and the java implementation to outlaw such objects, and offer users a migration path. just removing the tests in not OK, IMO.

@stale
Copy link

stale bot commented Oct 17, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Oct 17, 2019
@stale stale bot closed this Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove MapDoubleAliasExample
3 participants