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

Empty String "" Code Review Warning Annotation #31

Open
0x4007 opened this issue Mar 9, 2024 · 4 comments
Open

Empty String "" Code Review Warning Annotation #31

0x4007 opened this issue Mar 9, 2024 · 4 comments

Comments

@0x4007
Copy link
Member

0x4007 commented Mar 9, 2024

Annotate them with warnings in code review to more closely scrutinize the logic that interacts with the variable.

Rationale

I see a lot of abuse for empty strings. It makes code logic sloppy and more error prone. But since there are some legitimate uses for empty strings, we will just set a warning instead of error?

Examples

https://github.com/ubiquity/audit.ubq.fi/blob/08fba85a9a592ca2bda5eaeaa2ff3b9fdd4632be/static/scripts/audit-report/audit.ts#L26C1-L27C21
  • Should be unset (which implies undefined) instead of passing an empty string as a credential or URL. Then the code should check if the value is truthy before expecting authentication.
let REPOSITORY_URL;
let GITHUB_PAT;
https://github.com/ubiquity/ubiquibot-kernel/blame/c553d5a7866c1ef76d4f589d8095c8bb751cbd49/src/github/types/config.ts#L29
  • Should be value.ref ?? "@" + value.ref as I'm assuming the developer never expected the "" value to be returned there.
https://github.com/ubiquity/pay.ubq.fi/pull/189/files#r1518549017
  • This shouldn't have been here at all. It's not a valid RPC endpoint.

From codebase search: https://github.com/search?q=org%3Aubiquity+%5C%22%5C%22+language%3ATypeScript&type=code
@Keyrxng
Copy link
Member

Keyrxng commented Mar 25, 2024

/start

Copy link

ubiquibot bot commented Mar 25, 2024

DeadlineMon, Mar 25, 8:54 AM UTC
Registered Wallet 0xAe5D1F192013db889b1e2115A370aB133f359765
Tips:
  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

@Keyrxng
Copy link
Member

Keyrxng commented Jun 4, 2024

/stop if it is still an open task I will pick it back up. Feel free to ref my PR if anyone wants to take this

@ubiquibot ubiquibot bot unassigned Keyrxng Jun 4, 2024
Copy link

ubiquibot bot commented Jun 4, 2024

+ You have been unassigned from the task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants