Thriftpy / thriftpy2

Pure python approach of Apache Thrift.
MIT License
572 stars 91 forks source link

Support include files with dot in name #125

Closed aisk closed 4 years ago

aisk commented 4 years ago

Not a completed work, for now just support "one level include", like including a file called "com.example.api.thrift".

On the opposite, including a file called "com.example.thrift", which also including another file "api.thrift", is not supported now.

So this PR is marked as WIP, I'll try to finish this latter.

Close #121

codecov[bot] commented 4 years ago

Codecov Report

Merging #125 into master will decrease coverage by 3.32%. The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage   83.09%   79.77%   -3.33%     
==========================================
  Files          43       43              
  Lines        3911     3915       +4     
==========================================
- Hits         3250     3123     -127     
- Misses        661      792     +131     
Impacted Files Coverage Δ
thriftpy2/parser/parser.py 95.57% <94.11%> (+0.09%) :arrow_up:
thriftpy2/contrib/tracking/__init__.py 60.71% <0.00%> (-35.74%) :arrow_down:
thriftpy2/server.py 37.83% <0.00%> (-27.03%) :arrow_down:
thriftpy2/thrift.py 77.06% <0.00%> (-11.83%) :arrow_down:
thriftpy2/http.py 78.73% <0.00%> (-8.28%) :arrow_down:
thriftpy2/contrib/tracking/tracker.py 78.57% <0.00%> (-8.17%) :arrow_down:
thriftpy2/tornado.py 82.60% <0.00%> (-2.58%) :arrow_down:
thriftpy2/contrib/aio/processor.py 82.00% <0.00%> (-2.32%) :arrow_down:
thriftpy2/contrib/aio/socket.py 77.47% <0.00%> (-0.55%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b981e8c...62dd565. Read the comment docs.

aisk commented 4 years ago

The WIP label does not work, I found GitHub do not support this functionallity by default, but requires a app to be installed: https://github.com/marketplace/wip/ .

@ethe do you mind install it when you have time, I think the free plan is enough for open sourced project?

ethe commented 4 years ago

LGTM, I installed it.

aisk commented 4 years ago

Sorry I most forgot this PR. Although I think this PR is incomplete for implemented all the features, but this does fit what we need in company, and we run the codes in this PR for monthes (from when this PR created). So I think we can just merge it.

@ethe what do you think?