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(semver): prepare for noUncheckedIndexedAccess #4354

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

javihernant
Copy link
Contributor

ref #4040

@javihernant javihernant requested a review from kt3k as a code owner February 19, 2024 10:50
@@ -5,8 +5,8 @@ import { OPERATOR_XRANGE_REGEXP, XRANGE } from "./_shared.ts";
import { parseComparator } from "./_parse_comparator.ts";
import { parseBuild, parsePrerelease } from "./_shared.ts";

function isWildcard(id: string): boolean {
return !id || id.toLowerCase() === "x" || id === "*";
function isWildcard(id: string | undefined): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the optional ? assertion for option arguments instead of undefined union types. Ditto to other instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but ? is only available for functions whose required parameters don't follow an optional one. Meaning, I can change it in isWildcard(), but not in parseNumber() (_shared.ts)

semver/parse_range.ts Show resolved Hide resolved
@javihernant javihernant force-pushed the iss_4040_semver branch 2 times, most recently from 13a79ef to 56f193f Compare February 19, 2024 15:27
@javihernant
Copy link
Contributor Author

changed to using ? whenever possible. Also, provided justification for code that uses non-null assertion (ie. !). Let me know if there's anything else needed

Comment on lines 188 to 189
export function parseNumber(input: string | undefined, errorMessage: string) {
if (input === undefined) {
return 0;
}
const number = Number(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems wrong. Instead, I think we should use non-null assertions for the values we pass to the input argument. Even if the input is null, the function should fail, which is what we want.

@javihernant javihernant force-pushed the iss_4040_semver branch 2 times, most recently from 8ea975c to 4802477 Compare February 20, 2024 09:34
@javihernant javihernant requested a review from iuioiua February 20, 2024 09:35
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for this.

@iuioiua iuioiua changed the title refactor(semver): prepare for noUncheckedIndexedAccess refactor(semver): prepare for noUncheckedIndexedAccess Feb 20, 2024
@iuioiua iuioiua merged commit fe84d28 into denoland:main Feb 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants