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

Code Completion: Adding stub generator utility (and generated stub) #1939

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

KaylaHood
Copy link
Contributor

What does this change

The generate_stubs.py script creates a stub file, proxy.pyi, and saves it to the faker package's directory. This stub file contains extracted method declarations and docstrings (pulled from the providers) which are used by IDEs to provide code completion.

What was wrong

Because the provider classes are "dynamically" linked at runtime, IDEs can't get a list of available faker methods to display as code completion suggestions.

How this fixes it

Description of how the changes fix the issue.

Fixes #...
Generate a stub file (proxy.pyi) using the utility script (generate_stubs.py) to help IDEs provide code completion to developers.

@Viicos
Copy link
Contributor

Viicos commented Oct 25, 2023

Great! A couple things I saw:

  • It seems some typing imports are missing, e.g. Literal
  • It would be great to run black after the stub file is generated: we can use the API: black.format_str(..., mode=black.Mode(is_pyi=True))
  • I don't think attributes (such as credit_card_types) are available on the Faker instance at runtime
  • Do we want all the localized providers to be available, or only the methods from the provider in the __init__.py?

This can of course be discussed

@gryznar
Copy link

gryznar commented Dec 15, 2023

@KaylaHood @Viicos Is it going to happen something with this pr? Missing code completion is a big drawback :(

@KaylaHood
Copy link
Contributor Author

KaylaHood commented Dec 15, 2023

@KaylaHood @Viicos Is it going to happen something with this pr? Missing code completion is a big drawback :(

Sorry, I totally forgot to revisit this PR after I got the comment from Viicos.
@Viicos I have some questions:

  • Where are the typing imports missing? In the generate_stubs.py script?
  • How should I tie in the black API to the stub file generation? Would you like me to add a generate-stub recipe to the Makefile with the black recipe as a prereq?
  • Oops, you're right about the attributes not being accessible at runtime. I can fix that.
  • I think all of the localized providers should be available in the stub, because that's the most common usage of the faker library (most users don't select a subset of locales, they just use the Faker class as it comes by default). However, it is true that this can be confusing for situations where a specific locale/set of locales was given to the Faker initializer - the auto-complete will include methods that don't exist in the selected locale(s). I don't really see a way to avoid this without major refactoring of the Faker library (the locale selection of the Faker instance would need to be determinable prior to runtime).

@Viicos
Copy link
Contributor

Viicos commented Dec 15, 2023

Where are the typing imports missing? In the generate_stubs.py script?

Sorry, I meant in the generated stub file.

How should I tie in the black API to the stub file generation? Would you like me to add a generate-stub recipe to the Makefile with the black recipe as a prereq?

I can look into that.

I think all of the localized providers should be available in the stub, because that's the most common usage of the faker library.

Ah in that case it makes sense, I thought at first that only the non locale-specific methods were available. Maybe worth double checking this.


Other than that, the script looks good, but feels a bit hacky and could potentially lead to invalid syntax. I think a more robust implementation could be done using ast/cst, but that can be time consuming to implement. If I have time some day, I can come up with an implementation (but for later, sticking with your implementation is fine for now!)

@KaylaHood
Copy link
Contributor Author

I removed the attributes from the stub file and I added a recipe for generate-stubs to the Makefile. I also added it as a prereq to the release recipe. I didn't add black as a prereq because it wasn't already a prereq for the release recipe which implies it was intended as a "use-when-desired" recipe rather than an "always-run-prior-to-release" recipe.

Other than that, the script looks good, but feels a bit hacky and could potentially lead to invalid syntax. I think a more robust implementation could be done using ast/cst, but that can be time consuming to implement. If I have time some day, I can come up with an implementation (but for later, sticking with your implementation is fine for now!)

Yes, this was intended as a quick hack to get the autocomplete working for me while I wrote unit tests. I figured it'd be useful to the others on the issue thread who were having the same frustration. If you'd like to improve it, please be my guest!

@KaylaHood
Copy link
Contributor Author

Ah in that case it makes sense, I thought at first that only the non locale-specific methods were available. Maybe worth double checking this.

I made sure to include all the available locales in my stub generator (see lines 85-91)

@fcurella
Copy link
Collaborator

If the stubs are to be committed to the repo, then I would add generate-stubs to the lint recipe, rather than release.

We should alse update our contributing docs to mention running the task when submitting new PRs

…o adding ``make lint`` to the Contributing guide.
@KaylaHood
Copy link
Contributor Author

If the stubs are to be committed to the repo, then I would add generate-stubs to the lint recipe, rather than release.

We should alse update our contributing docs to mention running the task when submitting new PRs

Sorry for another delay, I was away on holiday. I've made the changes you suggested, can you check them out?

Copy link
Collaborator

@fcurella fcurella left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you!

@MarcelWilson
Copy link

MarcelWilson commented Jan 18, 2024

Thank you @KaylaHood!

Does it make sense to try to add a unittest which verifies all the expected methods are present in the stub?
There are also some missing imports in the stub file and mypy is throwing a few errors. Taking a look…

:param state_abbr: A state abbreviation
"""
...
def random_choices(self, elements: Union[Collection[str], Collection[T], collections.OrderedDict[T, float]] = ..., length: Optional[int] = ...) -> Sequence[T]:

Choose a reason for hiding this comment

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

Neither Collection nor collections appear in the imports

:sample: base='120004', number_system_digit=0, safe_mode=False
"""
...
def color(self, hue: Optional[HueType] = ..., luminosity: Optional[str] = ..., color_format: str = ...) -> str:

Choose a reason for hiding this comment

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

HueType isn't imported

:sample: size=(16, 16), hue=[90,270], image_format='ico'
"""
...
def json(self, data_columns: Optional[List] = ..., num_rows: int = ..., indent: Optional[int] = ..., cls: Optional[Type[json.encoder.JSONEncoder]] = ...) -> str:

Choose a reason for hiding this comment

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

json isn't imported

to that method.
"""
...
def credit_card_full(self, card_type: Optional[CardType] = ...) -> str:

Choose a reason for hiding this comment

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

CardType isn't imported

:example: datetime.time(15, 56, 56, 772876)
"""
...
def time_series(self, start_date: Union[datetime.date, datetime.datetime, datetime.timedelta, str, int] = ..., end_date: Union[datetime.date, datetime.datetime, datetime.timedelta, str, int] = ..., precision: Optional[float] = ..., distrib: Optional[Callable[[datetime.datetime], float]] = ..., tzinfo: Optional[datetime.tzinfo] = ...) -> Iterator[Tuple[datetime.datetime, Any]]:

Choose a reason for hiding this comment

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

Callable isn't imported

from faker.generator import Generator

T = TypeVar("T")
TEnum = TypeVar("TEnum", bound=Enum)

Choose a reason for hiding this comment

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

T and TEnum are redeclared after they are imported.

num_rows=10, include_row_ids=True
"""
...
def uuid4(self, cast_to: Union[Callable[[uuid.UUID], str], Callable[[uuid.UUID], bytes], None] = ...) -> Union[bytes, str, uuid.UUID]:

Choose a reason for hiding this comment

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

uuid isn't imported

MODULES_TO_FULLY_QUALIFY = ["datetime"]


imports: Dict[str, Optional[Set[str]]] = defaultdict(lambda: None)
Copy link

@MarcelWilson MarcelWilson Jan 18, 2024

Choose a reason for hiding this comment

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

Annotation should use DefaultDict instead of Dict
In fact, if you change the line to the following,

imports: DefaultDict[str, Set[str]] = defaultdict(set)

it will fix one of the mypy errors: Unsupported right operand type for in ("set[str] | None")

prov_cls, _, _ = Factory._find_provider_class(provider, locale)
classes_and_locales_to_use_for_stub.append((prov_cls, locale))

all_members: List[Tuple[UniqueMemberFunctionsAndVariables, str]] = \

Choose a reason for hiding this comment

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

Should be List[Tuple[UniqueMemberFunctionsAndVariables, Optional[str]]] since the last item in the list has None in it.

signatures_with_comments.append((f"def {func_name}{sig_str}: ...", None if comment == "" else comment, False))

signatures_with_comments_as_str = []
for sig, comment, is_preceding_comment in signatures_with_comments:

Choose a reason for hiding this comment

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

mypy errors because it infers sig is Signature from line 110 instead of str. Easiest solution would be to rename this variable.

new_parm = parm_val.replace(default=...)
if (new_parm.annotation is not inspect.Parameter.empty
and not new_parm.annotation.__module__ in BUILTIN_MODULES_TO_IGNORE):
module, member = get_module_and_member_to_import(new_parm.annotation, locale)

Choose a reason for hiding this comment

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

It looks like the reason a bunch of imports are missing is this function doesn't look inside annotations. So whenever Union[x, y] occurs, it doesn't grab x & y.

...
def __setstate__(self, state: Any) -> None: ...
def items(self) -> List[Tuple[str, Generator]]: ...
def seed(seed: Union[int, float, str, bytes, bytearray, None] = ...) -> None:

Choose a reason for hiding this comment

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

signature for this doesn't match. It's supposed to be a classmethod.

@MarcelWilson
Copy link

MarcelWilson commented Jan 19, 2024

I tweaked generate_stub.py to fix the issues with missing imports, staticmethods, and classmethods.
https://gist.github.com/MarcelWilson/bf830e853b54a59603013d50a801d948#file-generate_stub-py
Here is the stub output:
https://gist.github.com/MarcelWilson/ca6eaa52f91015c3bc2abbe5a442d6de#file-proxy-pyi

I took the liberty of removing the docstrings as they aren't needed in a stub. They can easily be re-enabled by flipping these lines:
https://gist.github.com/MarcelWilson/bf830e853b54a59603013d50a801d948#file-generate_stub-py-L161-L162

I'm not 100% it's covering all the methods without a unittest.

One final thought; it seems like it could be handy to create a stub for BaseProvider.generator for cases when referencing the methods in a new Provider. I'll take a look at that after this PR gets merged in.

@bandophahita
Copy link

bandophahita commented Jan 19, 2024

I went and made a branch with the changes I suggested above. (personal account vs work account). I even got the black and isort formatting included in the generate_stubs.py.
https://github.com/bandophahita/faker/tree/faker-stubs-rebased

If you'd like I can create a new PR with the updates.

@agusdmb
Copy link

agusdmb commented Mar 27, 2024

i've been following this PR, i would really like for it to make into master that's why im leaving this message.. having these stubs would be really helpful

@fcurella
Copy link
Collaborator

@KaylaHood do you want to ingrate @bandophahita's changes into your branch?

@KaylaHood
Copy link
Contributor Author

@fcurella I think I'd prefer to merge this PR without @bandophahita's changes and then have them make a new PR with their changes, which can be reviewed separately. This way he gets the recognition for his improvements.

@fcurella fcurella merged commit f96a1b1 into joke2k:master Mar 27, 2024
28 checks passed
@fcurella
Copy link
Collaborator

@bandophahita ball's in your court! 🙂

@bandophahita
Copy link

bandophahita commented Mar 27, 2024

@KaylaHood While I appreciate the gesture, I would say if it is easier feel free to integrate.

edit: missed that it was already merged. I'll make a PR

@RanaRana10
Copy link

i dont understand what did you say how i can active the type hint showing in vs code any tutorials or articles

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

Successfully merging this pull request may close these issues.

8 participants