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

🐛 Fix type conversion for List and Tuple and their internal types #143

Merged
merged 5 commits into from Jul 2, 2022

Conversation

hellowhistler
Copy link
Contributor

@hellowhistler hellowhistler commented Jul 16, 2020

Resolves #127 and an unticketed but related issue I found when digging through the code.

Fixes:

  • List parameters are properly converted to a list. Previously this worked for lists of Path and Enum but all other types of lists were converted to tuples.
  • Tuple parameters with Path or Enum elements will have those elements properly converted to their respective types. Previously this worked for all other types, but these two types were retained as strings. I removed a TODO item about recursive conversion which sounds like what I've done, but if that was referring to something else, I can re-add it.

@hellowhistler hellowhistler marked this pull request as ready for review July 16, 2020 23:21
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #143 (39c07ea) into master (b0fc0f0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #143   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          252       253    +1     
  Lines         5296      5340   +44     
=========================================
+ Hits          5296      5340   +44     
Impacted Files Coverage Δ
tests/test_others.py 100.00% <ø> (ø)
tests/test_type_conversion.py 100.00% <100.00%> (ø)
typer/main.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 b0fc0f0...39c07ea. Read the comment docs.

1. Tuple: ensure Enum and Path elements inside a tuple are converted to
   their respective types (rather than retained as strings)
2. List: ensure lists that contain non-Enum and non-Path elements are
   not inadvertently converted to tuples
@hellowhistler
Copy link
Contributor Author

Hi @tiangolo - would love to get your thoughts on this. I just pulled in the latest commits, made the code compliant with mypy --strict, and reworked the tests a little. Instead of putting them into the tutorial (which felt a bit like forcing a square peg), I moved them along with some other type conversion tests into a new file.

Copy link

@razy69 razy69 left a comment

Choose a reason for hiding this comment

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

Hi,

version: Typer==0.4.0

I experienced this issue in lazydocs project.
https://github.com/ml-tooling/lazydocs/blob/main/src/lazydocs/_cli.py#L33

Before this patch i had this error while trying to generate documentation.

'tuple' object has no attribute 'append'
Traceback (most recent call last):
  File "/Users/razy69/.pyenv/versions/3.9.6/envs/test39/lib/python3.9/site-packages/lazydocs/_cli.py", line 49, in generate
    generate_docs(
  File "/Users/razy69/.pyenv/versions/3.9.6/envs/test39/lib/python3.9/site-packages/pysnooper/tracer.py", line 263, in simple_wrapper
    return function(*args, **kwargs)
  File "/Users/razy69/.pyenv/versions/3.9.6/envs/test39/lib/python3.9/site-packages/lazydocs/generation.py", line 966, in generate_docs
    ignored_modules.append(module_name)

I used pysnooper to dig the issue, and it is clearly related to Typer arguments conversion.

$ lazydocs ./app --ignored-modules "main" --ignored-modules "tests" --overview-file README.md
Source path:... /Users/razy69/.pyenv/versions/3.9.6/envs/test39/lib/python3.9/site-packages/lazydocs/generation.py
Starting var:.. ignored_modules = ('main', 'tests')

I applied this patch, and it successfully fix issue.

$ lazydocs ./app --ignored-modules "main" --ignored-modules "tests" --overview-file README.md
Source path:... /Users/razy69/.pyenv/versions/3.9.6/envs/test39/lib/python3.9/site-packages/lazydocs/generation.py
Starting var:.. ignored_modules = ['main', 'tests']

@Michionlion
Copy link

@tiangolo Any update on possibly merging this fix? The workaround is minor (just add list(param) to get an actual list) but still an annoyance and potentially confusing. Seems like this PR is in pretty good working order. If there's any code changes that need to happen I'd be open to contributing myself, it'd be nice to have this merged.

@aperiodic
Copy link

aperiodic commented Mar 21, 2022

We were just bit by this bug ourselves. It would be nice to see this merged! Please let me know if there's any way I can assist.

@github-actions
Copy link

github-actions bot commented Jul 2, 2022

📝 Docs preview for commit 5eea0b3 at: https://62c07b152da60965a116c12a--typertiangolo.netlify.app

@tiangolo tiangolo changed the title Fix type conversion for certain kinds of multiple-value parameters 🐛 Fix type conversion for List and Tuple and their internal types Jul 2, 2022
@github-actions
Copy link

github-actions bot commented Jul 2, 2022

📝 Docs preview for commit 39c07ea at: https://62c07ed8f83fa36596f231ac--typertiangolo.netlify.app

@tiangolo
Copy link
Owner

tiangolo commented Jul 2, 2022

Awesome, thanks @hellowhistler! 🚀 🍰

And thanks everyone for the discussion here! ☕

This will be available in the next release, in some hours, Typer version 0.4.2 🎉

@tiangolo tiangolo merged commit 722a4fe into tiangolo:master Jul 2, 2022
zanieb added a commit to PrefectHQ/prefect that referenced this pull request Nov 11, 2022
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.

[BUG] Multiple value option gives tuple and not list
5 participants