thrift icon indicating copy to clipboard operation
thrift copied to clipboard

Published 20 hours ago • verified publisher icon apache

THRIFT-5139: THRIFT-4181: Python3 type hints

Open cspwizard opened this issue 3 years ago • 14 comments

Add type hinting and native enums (IntEnum) for python generated code

  • [x] Did you create an Apache Jira ticket? (not required for trivial changes)
  • [X] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • [X] Did you squash your changes to a single commit? (not required, but preferred)
  • [X] 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, include [skip ci] anywhere in the commit message to free up build resources.

cspwizard avatar Feb 16 '22 13:02 cspwizard

Cool! There's a PR here for enum changes: #2489

kainjow avatar Feb 19 '22 15:02 kainjow

Stupid question (probably): Can we make it the default or what reason does the switch have? Are there any potential incompatibilities? Just asking.

PS: you should rebase, theres's a conflict

Jens-G avatar Mar 06 '22 11:03 Jens-G

Just in case if somebody use lower versions of Python. However it make sense to make it default, and for older versions users can use the previous version. For me both ways are good, what is your opinion? @Jens-G

I'll rebase

cspwizard avatar Mar 06 '22 12:03 cspwizard

If there is a reason, leave it. I just want to make sure we don't introduce unnecessary switches. Seems not the case. As I said, stupid question :-)

Jens-G avatar Mar 06 '22 12:03 Jens-G

rebased and fixed a bug, for some reason on de/serialization it used enum variable as string

cspwizard avatar Mar 06 '22 13:03 cspwizard

What's blocking this from going forward?

ShaneEverittM avatar Nov 24 '22 22:11 ShaneEverittM

What's blocking this from going forward?

Pending change requests?

Jens-G avatar Nov 24 '22 23:11 Jens-G

Pending change requests?

what requests? I missed them probably

cspwizard avatar Nov 25 '22 06:11 cspwizard

@Jens-G , @ShaneEverittM are there any other comments or it could be completed?

cspwizard avatar Jan 14 '23 11:01 cspwizard

What remains to get this merged? It would be wonderful to see this land

pnovotnak avatar Mar 24 '23 18:03 pnovotnak

@jeking3 wrote:

There's more than just type hint changes in here, I found a case that I think will break many things including mock unit tests.

I'm generally in favour of this PR but I personally don't use py that much myself, thus given the comment above I'm not quite sure whether I am the right person to finally decide about the merge. Would be great if jeking could be re-review.

Jens-G avatar Mar 25 '23 09:03 Jens-G

@jeking3 Did you have any more feedback on this PR? Would love to have this! :D

roshanjrajan-zip avatar Jun 23 '23 20:06 roshanjrajan-zip

Will this PR be merged?

boringplay avatar Dec 03 '23 06:12 boringplay

This branch has conflicts that must be resolved Conflicting files compiler/cpp/src/thrift/generate/t_py_generator.cc

Jens-G avatar Dec 03 '23 20:12 Jens-G