-
Notifications
You must be signed in to change notification settings - Fork 116
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
chore: fix lint issues in src/share
#1384
base: develop
Are you sure you want to change the base?
Conversation
c68f445
to
4b34867
Compare
const contentSvg = (typeof possibleSvgContent === "object" | ||
&& possibleSvgContent !== null | ||
&& "svg" in possibleSvgContent | ||
&& typeof possibleSvgContent.svg === "string") | ||
? possibleSvgContent.svg | ||
: null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite hard to read, I would prefer extracting it in a method and using if
statements and writing a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean moving this as method into SNote class?
something like this?
It still looks ugly, but not much we can do about it I guess.
As I've noted in my commit message, using a schema validator like zod
would make life a lot easier for these types of situation (but that would be something for the future, not this PR I'd say).
snote.ts
getSvgContent() {
const possibleSvgContent = this.getJsonContentSafely();
return (typeof possibleSvgContent === "object"
&& possibleSvgContent !== null
&& "svg" in possibleSvgContent
&& typeof possibleSvgContent.svg === "string")
? possibleSvgContent.svg
: null;
}
routes.ts
const contentSvg = image.getSvgContent();
…any" in try/catch blocks
alternative solution, since they are unused and it is the last argument → remove it. We can still go that route later on though, if we agree upon it.
…ariable there's no need to assign a variable, if we never use the value outside of the if check
… type to have it as a named tuple
JSON and TS without using a validation library like zod, is really a bit of a pain in the backside...
4b34867
to
73305a5
Compare
WIP for fixing lint issues in
src/share
folder :-)Two errors will be left-over for now:
Two errors left:
this one is a bit weird -> even if I prepend it with an underscore (to mark it as deliberately unused) it flags it as an issue.
I want to avoid needing to add a "eslint disable" comment for that line for now, so I'll just keep it "open" for a final review after I went through all the other folders, running eslint, first
currently struggling about the correct way to make TypeScript happy with this "dynamic" way of accessing the Shaca class' properties.
In Becca there is an additional
if (!(camelCaseEntityName in this))
, but it doesn't really help TS with narrowing down the type unfortunately.will need to revisit later as well :-)