fix: sub module conflict error

[StellarisW]

Currently the logic of register sub module to sys.modules may cause conflict,

That's because of the PR #249 which fix #215

def load(path,
         module_name=None,
         include_dirs=None,
         include_dir=None,
         encoding='utf-8'):

    real_module = bool(module_name)
    thrift = parse(path, module_name, include_dirs=include_dirs,
                   include_dir=include_dir, encoding=encoding)
    if threadlocal.incomplete_type:
        fill_incomplete_ttype(thrift, thrift)

    if real_module:
        sys.modules[module_name] = thrift
        sub_modules = thrift.__thrift_meta__["includes"][:]
        while sub_modules:
            module = sub_modules.pop()
            if module not in sys.modules:
                sys.modules[module.__name__] = module
                sub_modules.extend(module.__thrift_meta__["includes"])
    return thrift

For example:

We have thrift files as follows:

thrift files
├── idl
│   ├── main.thrift
│   ├── included1.thrift
│   ├── included2.thrift

If we call load('idl/main.thrift', module_name='idl.main_thrift'), then the following modules will be registered:

• idl.main_thrift
• included1_thrift
• included2_thrift

So, if included1.thrift is a common module name like platform, which is platform.thrift, then module platform will register to sys.modules, so it will cause conflict.

Now I come up with 2 solutions:

1. Add an option to load(), which will be def load(path, module_name=None, include_dirs=None, include_dir=None, encoding='utf-8'), and let the users to register sub modules manually. (not recommended)

2. Optimize sub module name. (recommended)

   use absolute path as its module name instead of the base name of the thrift file.

   let's take the example before, then the following modules will be registered:

   • idl.main_thrift
   • idl.included1_thrift
   • idl.included2_thrift

[StellarisW]

Please take a review💌
@aisk @ethe

[aisk]

Sorry, I can't access my development computer these days. I will review it a few days later.

But the test case should be passed before merging.

[StellarisW]

Sorry, I can't access my development computer these days. I will review it a few days later.

But the test case should be passed before merging.

That's OK, I'll fix the test cases.

[cocolato]

just use s.replace(suffix, "") is better.

import itertools

from faker import Faker 

SUFFIX = "__SOME_SUFFIX_"

class MyFaker(Faker):
    def suffix_name(self):
        return f"{self.name()}{SUFFIX}"

fake = MyFaker()
data_num = 10000
escape_ratio = 0.5
escape_num = int(data_num * escape_ratio)

suffix_texts = [fake.suffix_name() for _ in range(escape_num)]
normal_texts = [fake.name() for _ in range(data_num - escape_num)]

def test_and_replace():
    for text in itertools.chain(suffix_texts, normal_texts):
        if text.endswith(SUFFIX):
            res = text.replace(SUFFIX, "")
    return res

def test_and_replace_with_slie():
    for text in itertools.chain(suffix_texts, normal_texts):
        if text.endswith(SUFFIX):
            res = text[:-len(SUFFIX)]
    return res

def just_replace():
    for text in itertools.chain(suffix_texts, normal_texts):
        res = text.replace(SUFFIX, "")
    return res


__benchmarks__ = [
    (test_and_replace, just_replace, "just_replace"),
    (test_and_replace, test_and_replace_with_slie, "test_and_replace_with_slie")
]

                                         Benchmarks, repeat=5, number=20                                          
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃                  Benchmark ┃ Min     ┃ Max     ┃ Mean    ┃ Min (+)         ┃ Max (+)         ┃ Mean (+)        ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│               just_replace │ 0.043   │ 0.043   │ 0.043   │ 0.021 (2.0x)    │ 0.022 (1.9x)    │ 0.022 (2.0x)    │
│ test_and_replace_with_slie │ 0.041   │ 0.042   │ 0.042   │ 0.040 (1.0x)    │ 0.041 (1.0x)    │ 0.040 (1.0x)    │
└────────────────────────────┴─────────┴─────────┴─────────┴─────────────────┴─────────────────┴─────────────────┘

[StellarisW]

Thanks for the advice, it seems better indeed.

[cocolato]

Suggested change

	sub_modules = thrift.__thrift_meta__["sub_modules"][:]
	for module in sub_modules:
	if module not in sys.modules:
	sys.modules[module.__name__] = include_thrift
	lost_modules = [
	m for m in thrift.__thrift_meta__["sub_modules"] if m not in sys.modules
	]
	for module in lost_modules:
	sys.modules[module.__name__] = include_thrift