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

refactor: make anys explicit #116

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

kalisjoshua
Copy link
Contributor

Closes #115

✅ Pull Request Checklist:

  • Included link to corresponding GitHub Issue.
  • The commit message follows conventional commit extended guidelines.
  • Added/updated unit tests and storybook for this change (for bug fixes / features).
  • Added/updated documentation (for bug fixes / features)
  • Filled out test instructions.

📝 Test Instructions:

❓ Does this PR introduce a breaking change?

  • Yes
  • No

💬 Other information

@kalisjoshua
Copy link
Contributor Author

This is just a suggestion to clean up the lint warnings. I am not passionate about this solution but I do like lint output to be clean.

@belsrc
Copy link
Contributor

belsrc commented Dec 3, 2024

I'm assuming they were just warnings, so not the end of the world. But I'm fine with sentiment. Overall though, I would prefer just targeted ignore comments over an alias.

@brandonjpierce
Copy link
Contributor

+1 to Bryan, sentiment is totally sound, but I do hesitate to obfuscate a baseline TS type. The larger question is actually how this warning/error would be handled in other linters e.g. eslint and oxc. Are they perhaps "smarter" about understanding the alias just points to any and you will still receive a warning, etc.

@kalisjoshua
Copy link
Contributor Author

Yeah, I am not a big fan either. I just was floating it out there because I was being lazy and didn't want to litter // biome-ignore... all over the place. I've had a setup in the past that would allow explicit any and disallow implicit any. Feelings?

@belsrc
Copy link
Contributor

belsrc commented Dec 3, 2024

I've had a setup in the past that would allow explicit any and disallow implicit any.

Interesting thought. My feeling is that it could be "abused." In the sense that there is nothing stopping someone from anying everything as long as it is explicit. Which is just more work for all parties involved.

@brandonjpierce
Copy link
Contributor

Yeah, I am not a big fan either. I just was floating it out there because I was being lazy and didn't want to litter // biome-ignore... all over the place. I've had a setup in the past that would allow explicit any and disallow implicit any. Feelings?

I'm actually for this if we can make sure the various linters could all theoretically support it

@kalisjoshua
Copy link
Contributor Author

I'm fine with this approach till it doesn't work. I just don't like to see the warnings in the output.

@kalisjoshua
Copy link
Contributor Author

I also prefer this to ignore statements all over the place; but I only prefer it 51% to 49%, so am easily swayed.

@kalisjoshua kalisjoshua force-pushed the refactor/115-cleaner-lint branch from 838d9ff to ebc4e4f Compare December 3, 2024 18:47
@belsrc
Copy link
Contributor

belsrc commented Dec 3, 2024

I also prefer this to ignore statements all over the place; but I only prefer it 51% to 49%, so am easily swayed.

Lets see if I can articulate what is in my head, since eloquence isn't a strong suit of mine. Allowing any shifts where the "burden" is. With just any all of the burden is then on the reviewer to make sure that everything is correct. With "no explicit any" then it is more on the contributor. They are not allowed to use any so they are forced to make a type correct. If they fallback to a comment to disable, there should be a reason (as all biome comments allow with the -ignore: <reason>). Which forces them to justify the reason then and there. And it reduces the work on the reviewer as seeing a comment that doesn't have the reason is an immediate flag, full stop. Seeing a reason, you can get the contributors rationale immediately and can go from there.

@kalisjoshua
Copy link
Contributor Author

I can appreciate the desire for more eloquence as remote communication is always more difficult.

It sounds like you interpreted my comment as me wanting to not allow using any explicitly; this is not the case. I am fine with any in key areas and when necessary; these will always exist.

I think we are also aligned with where the "burden" of responsibility should reside; on the author not the reviewer. The author should be responsible for clearly describing the changes they are introducing both in implementation logic and type definition.

The use of any should be avoided as often as possible as it doesn't help with understanding what the shapes of values should be.

There are far more uses of // biome-ignore ... without a description filled out than with one filled out.

What I was suggesting is allow explicit any (without a warning) and disallow implicit any (as an error).

@belsrc
Copy link
Contributor

belsrc commented Dec 3, 2024

What I was suggesting is allow explicit any (without a warning) and disallow implicit any (as an error).

That is exactly what I am referring to, though. When I said any above, I mean explicit any (with no warn).

There are far more uses of // biome-ignore ... without a description filled out than with one filled out.

I would have to have you argue your case for that. As, in my eyes, its no different then @ts-x <reason>. Both should only be used when there is no other option. And their use should be justified. It is kind of a disservice to future maintainers (which may be oneself) if there is no reason provided. As no one may know/remember why something is there in the first place.

@brandonjpierce
Copy link
Contributor

I think the largest value add for explicit @ts-expect-error or biome-ignore is really for that additional description/reason context for why the any is required. I've gone back through so many old codebases and just thought to myself "why the heck didn't I ever fix this?" only to slowly remember why 😆

@kalisjoshua kalisjoshua force-pushed the refactor/115-cleaner-lint branch from 9d934f9 to a8d3b2a Compare December 4, 2024 18:05
@kalisjoshua
Copy link
Contributor Author

I would have to have you argue your case ...

I am not making a case for anything, I am stating a fact. Search for // biome-ignore lint/suspicious/noExplicitAny across the code base on "main" and there are more instances without an explanation than there are with one. Broadening the search to just // biome-ignore and analyzing the results shows that there are more instances without than there are with; and of the ones that have something very few are actually explaining a reason for the exception.

I am not trying to be critical or getting on a high horse; because I am also guilty of not doing a good job of filling in an explanation myself. I am just pointing out that there does not seem to be rigor in requiring the explanation currently.

What I would argue is that disabling the warning for explicit use of any would be cleaner than forcing us to add the ignore comment for each instance. It would also be less work as disabling it would be effort once and keeping it as a warning requires ignoring continuously into the future.

@kalisjoshua kalisjoshua force-pushed the refactor/115-cleaner-lint branch from a8d3b2a to 6084f41 Compare December 4, 2024 18:37
@belsrc
Copy link
Contributor

belsrc commented Dec 4, 2024

Going in circles.

What I would argue is that disabling the warning for explicit use of any would be cleaner than forcing us to add the ignore comment for each instance. It would also be less work as disabling it would be effort once and keeping it as a warning requires ignoring continuously into the future.

👉 #116 (comment)

I am not trying to be critical or getting on a high horse; because I am also guilty of not doing a good job of filling in an explanation myself. I am just pointing out that there does not seem to be rigor in requiring the explanation currently.

That is very much an "us" thing and in no way invalidates the point. I don't know where the <explanation> stuff is coming from (VSC crap?) but if you omit it, biome already tells you to do better.

Screenshot 2024-12-04 at 2 56 48 PM

@kalisjoshua
Copy link
Contributor Author

Ok good. Then I was just misunderstanding.

So we want to change the setting to allow for explicit any without an disable comment required?

@kalisjoshua kalisjoshua force-pushed the refactor/115-cleaner-lint branch from 6084f41 to 598978c Compare December 4, 2024 20:35
@kalisjoshua kalisjoshua force-pushed the refactor/115-cleaner-lint branch from 598978c to d7867f0 Compare December 4, 2024 21:12
@brandonjpierce
Copy link
Contributor

brandonjpierce commented Dec 11, 2024

@kalisjoshua TL;DR:

  • We keep the rule for no any (error)
  • IF you need an any, we require an explicit biome-ignore comment including description to indicate the nuance of why we need it
  • The scope of this ticket really just becomes adding in the biome-ignore to the @accelint/core package (where I think most of the anys exist) and removing the eslint-ignore comments (like you had already did)

This way, any any that exists in the codebase, should have an appropriate context attached to it vs. some may or may not having it with the explicit any rule

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.

refactor: Clean up linter warnings
3 participants