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

[FEATURE]: Add typechecking for codebase #1161

Merged
merged 1 commit into from
Nov 14, 2023
Merged

[FEATURE]: Add typechecking for codebase #1161

merged 1 commit into from
Nov 14, 2023

Conversation

vhyrro
Copy link
Member

@vhyrro vhyrro commented Nov 14, 2023

closes #1160

This PR adds a github workflow which verifies the types of all Neorg's code. I expect it to complain quite a lot so the next step will be to clean up all of the type errors it throws :p

Thanks to @pysan3 for the idea!

@vhyrro
Copy link
Member Author

vhyrro commented Nov 14, 2023

And indeed it throws a lot of errors. The next logical step will be to go in and clean it all up one by one in another PR! If anyone's happy to also help out with this then feel free to make your own pull requests :)

@vhyrro vhyrro merged commit 8327957 into main Nov 14, 2023
2 of 4 checks passed
@vhyrro vhyrro deleted the typecheck-codebase branch November 14, 2023 17:16
@pysan3
Copy link
Contributor

pysan3 commented Nov 14, 2023

Hey @vhyrro , looks like a good step forward.

I am definitely willing to help out but this will require somewhat big rewrite of many code, which will definitely make tons of conflicts between other PRs and other ongoing works.

Should I start off with modules that have smaller impact (do you have any suggestion?) and make PR bit by bit (maybe module by module?), or should I make a giant PR and experience the resolving conflict pain only once?

@pysan3
Copy link
Contributor

pysan3 commented Nov 14, 2023

@vhyrro people might be afraid of the ❌, so shall we add ---@diagnostic disable-line for all places where the test is failing as a temporary solution?

If yes I'll make a PR tomorrow at the earliest.

@vhyrro
Copy link
Member Author

vhyrro commented Nov 14, 2023

Hey @vhyrro , looks like a good step forward.

I am definitely willing to help out but this will require somewhat big rewrite of many code, which will definitely make tons of conflicts between other PRs and other ongoing works.

Should I start off with modules that have smaller impact (do you have any suggestion?) and make PR bit by bit (maybe module by module?), or should I make a giant PR and experience the resolving conflict pain only once?

Most of the changes should be minor things like checking types (type(var) == "XYZ"), adding type annotations or adding nil checks. I don't expect those to cause large merge conflicts so I'd rather many small PRs with a few fixes each :)

@vhyrro
Copy link
Member Author

vhyrro commented Nov 14, 2023

@vhyrro people might be afraid of the ❌, so shall we add ---@diagnostic disable-line for all places where the test is failing as a temporary solution?

If yes I'll make a PR tomorrow at the earliest.

We could do that yeah. Perhaps parse the json that the github action generates and just write a script that adds that line everywhere, then go through each annotation bit by bit.

@Anrock
Copy link

Anrock commented Nov 15, 2023

From my experience of introducing style / type / lint / diagnostic checks to whole (non-confirming) database the best way was to add the checker with corresponding check steps in pipeline and simultaneously put every checkable code file into global ignore file (effectively making the check noop) in the first PR and then fix and take out modules/files one by one from ignore file in multiple separate subsequent PRs. This way has almost no friction for finishing and merging existing PRs. If some friction is okay: the first step shouldn't put files into ignore list if they're green initially.

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.

Proposal: Use lua-typecheck-action for all PRs
3 participants