-
Notifications
You must be signed in to change notification settings - Fork 6
try to refine type #38
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
base: master
Are you sure you want to change the base?
try to refine type #38
Conversation
by checking for subclasses
|
This seems like a reasonable approach. I do have some feedback, however. |
|
Out of interest, what is an example of this PR improving behaviour? I would have though your example in #37 would predict If I ever rewrite my type system (and the |
|
|
||
| return infer_type(self.description) or cli_types.CliString() | ||
| tpe = None | ||
| tpe_cand = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer descriptive variable names where possible. typ and type_candidates please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have though your example in #37 would predict None, then CliFile() then CliFile() respectively, and then return CliFile(). We only use subclasses for CliFileSystemType, and only recently, but the inference will never return CliFileSystemType, so it seems irrelevant.
True. I already forgot that we implemented the distinction between in/output files/dirs by properties and not subclasses.Then I will check for isinstance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a test case also please. You should be able to use the exact example from the linked issue.
Can try. Do you have an example how this is done? Or where? |
since in/out file/dir is represented by properties of the same class - also fixes linter errors
|
Ah well eventually I'll make a |
|
Also look into getting pip install pre-commit
pre-commit install |
by checking for subclasses