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 ts files parser utils #756

Merged
merged 6 commits into from
Jan 26, 2024

Conversation

prajwalkulkarni
Copy link
Contributor

@prajwalkulkarni prajwalkulkarni commented Jan 22, 2024

💫 Changelog

⭐ Improved typesafety in TS files located in parser/utils.
⭐ Changed var to let and const as applicable.
⭐ Moved constant value identifiers outside of function definition to avoid unnecessary re-initialization.
⭐ Added strict equality check

@prajwalkulkarni
Copy link
Contributor Author

I aim to improve the type-safety across the complete codebase in iterations. In this PR, the type of a few properties in Atom interface are still defined as any(and at few a places the types are also asserted as any, e.g: In MMTF.ts) as I'm still wrapping my head around the codebase and the type of the properties will be updated in further iterations.

8/13 files of parsers/utils are refactored in this PR, kindly confirm if I should push the refactored changes of the remaining 5 files of parsers/utils in this PR or create a separate PR. Thanks!

let atoms = [];
// interface Atoms {index: number; atom: string; hbondDistanceSq: number; hbondOther: any; hetflag:any}

export interface Atom {
Copy link
Contributor

Choose a reason for hiding this comment

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

AtomSpec is already defined in spec.ts. Please do not define a duplicate type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you pointing it out, I was not aware of it mostly because it was not used anywhere in utils.
Additionally, I have also added some of the missing types in AtomSpec. Currently, all the properties are optional, which shouldn't be the case, but as I further refactor other files and get better types for defining the properties, I'll update them in subsequent commits/PRs

src/parsers/utils/areConnected.ts Outdated Show resolved Hide resolved
}
if (elem.length > 1) {
elem = elem[0].toUpperCase() + elem.substring(1).toLowerCase();
if (typeof bondTable[elem] === "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not directly compare to undefined? The typeof comparison is left over from javascript where undefined was not a reserved keyword (not true in TypeScript).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe undefined has always been a primitive type in JS(reserved keyword). Nonetheless, I've updated the condition(s) to compare to undefined directly instead of using typeof (I might as well want to make these changes in the other PR #754, I will let you know once I push the changes there).

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot the other reason to use typeof is it doesn't throw an error if the thing doesn't exist. So this should be replaced with direct comparisons to undefined with care.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, you are right. If an undeclared variable is compared to an undefined primitive, it would throw an error whereas typeof would safely return a boolean.

@dkoes dkoes merged commit 8c3e9d4 into 3dmol:master Jan 26, 2024
5 checks passed
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