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

Some improvement? #1

Open
zw963 opened this issue Dec 8, 2024 · 1 comment
Open

Some improvement? #1

zw963 opened this issue Dec 8, 2024 · 1 comment

Comments

@zw963
Copy link

zw963 commented Dec 8, 2024

  1. If we know a arg type is nilable with only one type, e.g. String | Nil, can we replace it with String? ?

  2. We don't need add the parentheses for union type? e.g. instead of (String | Int32), we can replace it with: String | Int32.

  3. If user don't know how to use this tool, so, run just ./typer alone is possible.
    This give a error message like this:

 ╰──➤ $ bin/typer
Unhandled exception: Index out of bounds (IndexError)
  from /home/zw963/Crystal/share/crystal/src/array.cr:1512:13 in 'shift'
  from lib/source-typer/src/cli_options.cr:56:19 in 'parse'
  from lib/source-typer/src/cli.cr:5:1 in '__crystal_main'
  from /home/zw963/Crystal/share/crystal/src/crystal/main.cr:118:5 in 'main_user_code'
  from /home/zw963/Crystal/share/crystal/src/crystal/main.cr:104:7 in 'main'
  from /home/zw963/Crystal/share/crystal/src/crystal/main.cr:130:3 in 'main'
  from /usr/lib/libc.so.6 in '??'
  from /usr/lib/libc.so.6 in '__libc_start_main'
  from bin/typer in '_start'
  from ???

We can print the help message instead this exception.

If you prefer those improve, i can try make a PR for several of them.

Thanks.

@Vici37
Copy link
Owner

Vici37 commented Dec 8, 2024

Aha! Yeah, great suggestions! I've actually incorporated both the first and second bullets into my port into the crystal compiler itself 😅 The third point is still a bug that I missed, whoops.

If the crystal team doesn't accept the new tool, then I'll backport those changes into here. We'll see. I'll keep this issue open until that gets decided.

You can find that PR here.

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

No branches or pull requests

2 participants