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

Add Typing/MacroCallArgumentTypeRestriction #521

Conversation

nobodywasishere
Copy link
Contributor

No description provided.

@Sija Sija added the rule label Dec 3, 2024
@Sija Sija added this to the 1.7.0 milestone Dec 3, 2024
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

LGTM. Nice one

Copy link
Member

@Sija Sija left a comment

Choose a reason for hiding this comment

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

Aside of the review comments, I'm missing here a default_value rule option, which would exclude the cases where the default value is given.

src/ameba/rule/typing/macro_call_var_type_restriction.cr Outdated Show resolved Hide resolved
src/ameba/rule/typing/macro_call_var_type_restriction.cr Outdated Show resolved Hide resolved
src/ameba/rule/typing/macro_call_var_type_restriction.cr Outdated Show resolved Hide resolved
@Sija
Copy link
Member

Sija commented Dec 29, 2024

Also, I believe sth like a MacroCallArgumentTypeRestriction would fit better as the rule name.

@nobodywasishere nobodywasishere changed the title Add Typing/MacroCallVarTypeRestriction Add Typing/MacroCallArgumentTypeRestriction Dec 29, 2024
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
@Sija
Copy link
Member

Sija commented Dec 30, 2024

At this point the record macro IMO doesn't seem to fit into this, since:

  1. It doesn't accept most of the argument types
  2. With the Assign nodes, the argument type is still inferred

@nobodywasishere
Copy link
Contributor Author

It doesn't accept most of the argument types

I don't see this as an issue. It accepts Assign, which is the majority of uses of all of these macro calls.

With the Assign nodes, the argument type is still inferred

This series of PRs is explicitly about removing this inference, so I disagree and think that the record macro still fits.

@veelenga veelenga self-requested a review December 30, 2024 08:14
@Sija Sija requested review from veelenga and removed request for veelenga January 8, 2025 07:21
@veelenga
Copy link
Member

veelenga commented Jan 9, 2025

I agree with @nobodywasishere on keeping record in the list. But would it make sense to default default_value to false? While typing restrictions are helpful, I think the primary goal is readability, and sometimes type inference just does the job. For example, this is totally fine

record Task,
  cmd = "ameba",
  args = [] of String

and we don't have to enforce this by default:

record Task,
  cmd : String = "ameba",
  args : Array(String) = [] of String

@nobodywasishere
Copy link
Contributor Author

@veelenga That's how I should've done it (and how I did it in the other typing rules), where when enabled, it's the least-restrictive form of the rule, with the option to go more restrictive.

I think the primary goal is readability

A secondary goal for me is in tooling, where I will probably use this and other rules to enforce types so a language server has an easier time figuring out what the types of things are.

@Sija Sija merged commit 65f7db0 into crystal-ameba:master Jan 9, 2025
4 checks passed
@nobodywasishere nobodywasishere deleted the nobody-macro-call-var-type-restriction branch January 9, 2025 16:09
@Sija Sija linked an issue Jan 9, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rules for enforcing type restrictions on vars and parameters
3 participants