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

THRIFT-4951 Remove tests for deprecated senum base type #1868

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

Issacpeng
Copy link
Contributor

@Issacpeng Issacpeng commented Sep 6, 2019

clean up unsupported senum base type from 0.9.1

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, add [skip ci] at the end of your pull request to free up build resources.

clean up unsupported senum base type from 0.9.1
@dcelasun dcelasun merged commit df8ef4b into apache:master Oct 9, 2019
@Jens-G
Copy link
Member

Jens-G commented Oct 9, 2019

But sernum is not unsupported? It is only deprecated.

@Jens-G
Copy link
Member

Jens-G commented Oct 9, 2019

I might not have been clear about this, but I'm against removing a working test for a still existing, although deprecated, feature. What added value does converting a tested feature into an untested feature provide?

IMHO this one should be reverted and should have been closed from the beginning.

@Jens-G Jens-G changed the title THRIFT-4951 clean up unsupported senum base type from 0.9.1 THRIFT-4951 Remove tests for deprecated senum base type Oct 9, 2019
@dcelasun
Copy link
Member

dcelasun commented Oct 9, 2019

But sernum is not unsupported? It is only deprecated.

Sorry, I thought it was unsupported. I'd be happy to revert it, but perhaps it's a good time to retire senum for good? It's been more than 5 years since the deprecation. We should either remove it or un-deprecate it.

@Jens-G
Copy link
Member

Jens-G commented Oct 9, 2019

Agree, but as I wrote in JIRA we should first do our homework and check if fbthrift still supports it, for sake of compatibility. Could be worth filing an fbthrift ticket, if they still have it, so we get rid of it on both ends. And while we're at it, we could also do the same for slist

@Issacpeng
Copy link
Contributor Author

Issacpeng commented Oct 14, 2019

@Jens-G @dcelasun i double checked senum and slist in fbthrift, find 2 remove commits, hope it can works for us~

1.senum: facebook/fbthrift@e03dab8
2.slist:facebook/fbthrift@9681401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants