[apply-annotations] Add argument for ignoring existing annotations.

[pradeep90]

Summary

This will allow us to override existing types based on the stub.

Test Plan

Added test case.

[codecov-io]

Codecov Report

Merging #291 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #291   +/-   ##
=======================================
  Coverage   93.93%   93.93%           
=======================================
  Files         220      220           
  Lines       21307    21318   +11     
=======================================
+ Hits        20014    20025   +11     
  Misses       1293     1293           

Impacted Files	Coverage Δ
libcst/codemod/visitors/_apply_type_annotations.py	96.16% <100.00%> (+0.04%)	⬆️
...emod/visitors/tests/test_apply_type_annotations.py	100.00% <100.00%> (ø)

---

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 940647e...4bcef66. Read the comment docs.

[zsol]

Functionality looks good to me. I have one nit: ignore_existing_annotations is not expressive enough IMO. I'd be much happier with something like overwrite_existing_annotations. Wdyt?

[jimmylai]

Functionality looks good to me. I have one nit: ignore_existing_annotations is not expressive enough IMO. I'd be much happier with something like overwrite_existing_annotations. Wdyt?

Agree. overwrite_existing_annotations is more direct.

[jimmylai]

Can we add overwrite_existing_annotations to the static function add_stub_to_context (can add it to both)?
So overwrite_existing_annotations is stored in the context (probably use a different key).
That can make it easier for user since in some codemod application, user doesn't need to explicitly create the visitor instance and it's done by the high level Codemod helper.
E.g. In this codemod, user just calls the static method to add needed imports without needed to create AddImportsVisitor instance.

LibCST/libcst/codemod/commands/strip_strings_from_types.py

Line 45 in 379c9d0

AddImportsVisitor.add_needed_import(self.context, "__future__", "annotations")

[pradeep90]

Yeah, added the argument to the static method as well.

Design choice: Setting the argument to True in any call will overwrite annotations for all stubs. The alternative was to store a flag per stub (and thus per annotation), which seemed like overkill.

[pradeep90]

Found that API a bit confusing since it wasn't clear what would happen if you called add_stub_to_context(stub1, overwrite_existing_annotations=True) and add_stub_to_context(stub2, overwrite_existing_annotations=False).

Decided to go with a separate static method set_overwrite_existing_annotations_flag_in_context such that calling it at any point will set the flag to True. (It does not accept a bool flag to avoid the ambiguity I mentioned above with multiple calls.)

[jimmylai]

Do you expect user calls ApplyTypeAnnotationsVisitor.add_stub_to_context multiple times?
The function takes a stub module and apply the stub on current module. When you run over a list of stub files, you'll need to call add_stub_to_context with different stub module and context. Separate set_overwrite_existing_annotations_flag_in_context as another staticmethod adds one more step to config the overwrite which doesn't seem make it easier to use due to the overwrite flag is stored in the context. That means each module needs to call set_overwrite_existing_annotations_flag_in_context separately if they want to enable overwrite.

[pradeep90]

Do you expect user calls ApplyTypeAnnotationsVisitor.add_stub_to_context multiple times?

Yes, as it stands, add_stub_to_context can be called multiple times to add annotations for the same module, just like add_needed_import. That is what creates the ambiguity I pointed out above.

Decided to store only one stub in the context instead of maintaining a list, i.e., ApplyTypeAnnotationsVisitor.store_stub_in_context(context, stub, overwrite_existing_annotations). This is more limited than the option of adding multiple stubs for the same module, but is less ambiguous and matches the notion of one stub per Python source file. Let me know if that works.