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: minor code improvements #4

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

jobveldhuis
Copy link
Contributor

Thanks for this plugin, @LuudJanssen. In this PR, I have made some minor code adjustments to improve readability and increase the semantic value of the code. These changes can roughly be split into three different subjects, which I will outline, below:

Swap returning false for returning null when no result can be returned

Some functions return either a result or false when no result can be provided. I do not think this is semantically correct, as I expect false to be returned only when in a type guard or another assertion function. When a function parses and returns data, I expect either an error to be raised or null to be returned as a value to signal the parsing was completed successfully, but no valid result could be retrieved from parsing, therefore returning null.

Change is-github-alert-declaration.ts to parse-github-alert-declaration.ts

The function contained in this file was called parseGithubAlertDeclaration. I believe file names should follow function naming and have changed this in this PR.

Add type guard function to check whether provided type is a GitHub alert type

Before this PR, we needed to typecast the type variable in parseGithubAlertDeclaration twice. Once to check whether the type was correct and twice when returning. I have added a utility to check whether a type is a GitHub alert type, which borrows its logic directly from the previous code. The second advantage of stripping this logic into a separate function is that we can test that function directly and ensuring that, during parsing, only the correct types are being parsed - without having to test the flow of multiple functions.

@jobveldhuis jobveldhuis changed the title Minor code improvements refactor: Minor code improvements Apr 11, 2024
@jobveldhuis jobveldhuis changed the title refactor: Minor code improvements refactor: minor code improvements Apr 11, 2024
Copy link
Contributor

@LuudJanssen LuudJanssen left a comment

Choose a reason for hiding this comment

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

Thanks @jobveldhuis, LGTM!

@LuudJanssen LuudJanssen merged commit 965b637 into incentro-ecx:main Apr 18, 2024
4 checks passed
@LuudJanssen
Copy link
Contributor

Released in v1.0.5.

@jobveldhuis minor comment, but your commit prefix should not start with feat if it does not actually add any functionality. refactor would have been most appropriate here I think. (It tripped up auto-versioning based on commit messages.)

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