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

Adress issue with intern not working #725

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Dec 1, 2023

Hi,

could you please review patch to fix intern. It addresses #724.

It was using var methods such as .set-root that don't exist.

Tests included.

Thanks

chrisrink10
chrisrink10 previously approved these changes Dec 4, 2023
Copy link
Member

@chrisrink10 chrisrink10 left a comment

Choose a reason for hiding this comment

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

Looks like MyPy is saying we have some # type: ignore comments which are no longer needed, but the change otherwise looks good to me.

@ikappaki
Copy link
Contributor Author

ikappaki commented Dec 4, 2023

Looks like MyPy is saying we have some # type: ignore comments which are no longer needed, but the change otherwise looks good to me.

Thanks, I've corrected all these side errors in #723. Would you like me to back port them in here for the PR to pass CI? I was hoping #723 would go first, but I guess it is much to go through :)

@chrisrink10
Copy link
Member

Looks like MyPy is saying we have some # type: ignore comments which are no longer needed, but the change otherwise looks good to me.

Thanks, I've corrected all these side errors in #723. Would you like me to back port them in here for the PR to pass CI? I was hoping #723 would go first, but I guess it is much to go through :)

Ah yeah, I haven't had a chance to go through that yet. If it's not too much, you could pull in at least the 2 changes that are causing MyPy to error here. I will try to take a look at the other one this week if I can.

@ikappaki
Copy link
Contributor Author

ikappaki commented Dec 4, 2023

If it's not too much, you could pull in at least the 2 changes that are causing MyPy to error here. I will try to take a look at the other one this week if I can.

Done, thanks

chrisrink10
chrisrink10 previously approved these changes Dec 4, 2023
Copy link
Member

@chrisrink10 chrisrink10 left a comment

Choose a reason for hiding this comment

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

Sorry I merged your other PR and created a conflict on the changelog. I also think you can just simplify the # type: ignore comments for most cases by just removing the comments.

@@ -3527,7 +3527,7 @@ def _const_type_to_py_ast(form: IType, ctx: GeneratorContext) -> GeneratedPyAST:

ctor_args = []
ctor_arg_deps: List[ast.AST] = []
for field in attr.fields(tp): # type: ignore[arg-type]
for field in attr.fields(tp): # type: ignore[arg-type, misc, unused-ignore]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for field in attr.fields(tp): # type: ignore[arg-type, misc, unused-ignore]
for field in attr.fields(tp):

I think you can just do this rather than adding unused-ignore code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately ignore by itself does not suffice if there is an unused-ignore error reported in some python versions (e.g. 3.8) but not others (3.11), it has to be added to the list:

please see below how type: ignore reports an unused-ignore error for runtime.py in 3.8 but not in 3.11.

3.8:

py38-mypy: commands[0]> mypy --config-file=C:\src\basilisp/pyproject.toml -p basilisp
src\basilisp\lang\runtime.py:1958: error: Unused "type: ignore" comment  [unused-ignore]
src\basilisp\lang\compiler\generator.py:3530: error: Unused "type: ignore[misc]" comment  [unused-ignore]
src\basilisp\lang\compiler\analyzer.py:336: error: Unused "type: ignore" comment  [unused-ignore]
Found 3 errors in 3 files (checked 47 source files)

3.11

py311-mypy: commands[0]> mypy --config-file=C:\src\basilisp/pyproject.toml -p basilisp
src\basilisp\lang\runtime.py:1958: error: Argument 1 to "intern" of "Var" has incompatible type "Namespace | None"; expected "Namespace | Symbol"  [arg-type]

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's frustrating for us. Thanks for the explanation.

@ikappaki
Copy link
Contributor Author

ikappaki commented Dec 5, 2023

Sorry I merged your other PR and created a conflict on the changelog. I also think you can just simplify the # type: ignore comments for most cases by just removing the comments.

I've rebased from main resolving the changelog conflict.

@chrisrink10 chrisrink10 merged commit 63ba73f into basilisp-lang:main Dec 5, 2023
7 checks passed
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.

2 participants