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

TS data verification #39

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

TS data verification #39

wants to merge 38 commits into from

Conversation

Plymatea
Copy link

Ticket:

  • None

What this Does:

  • Adds Read Only verification to tests, methods, and properties
  • Files should all ready have TS verification for method parameters, and output.

Media:
None

Testing

  • All jest test still pass

Plymatea and others added 30 commits June 6, 2022 16:56
"merge updates to class into testing branch before PR is complete"
@Plymatea Plymatea requested a review from DuHerb June 13, 2022 19:12
@Plymatea Plymatea changed the title Add ReadOnly TS type verification TS data verification Jun 13, 2022
@@ -0,0 +1,29 @@
{
Copy link

Choose a reason for hiding this comment

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

this launch.json file should be listed in your .gitignore file

Copy link
Author

Choose a reason for hiding this comment

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

Done, I don't think I understand git versioning well enough to scrub the file from the git repo history. I did it once on a personal project, but scrubbed the entire repo. I'm afraid I'll screw something up if I try it here.

I did add the /vscode folder the gitignore though.

src/index.ts Outdated
name: string
favColor: string
age: number
Copy link

Choose a reason for hiding this comment

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

all properties here should be readonly

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/index.ts Outdated
private _users: Readonly<Record<string, IUser>>

constructor(seedData: Record<string, IUser> = {}) {
Copy link

@DuHerb DuHerb Jun 14, 2022

Choose a reason for hiding this comment

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

Wouldn't be a bad thing to define/export an IUsers type. type IUser = Record<string, IUser>

only suggested here because we use this list as a type in several places.

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/index.ts Outdated
}

addUser(user: IUser): IUser{
Copy link

Choose a reason for hiding this comment

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

Suggested change
addUser(user: IUser): IUser{
addUser(user: IUser): IUser {

space ^^

Copy link

Choose a reason for hiding this comment

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

looks like space is missing after return types on several methods. should fix

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/index.ts Outdated
}

searchUserByName(name: string): Array<IUser>{
Copy link

Choose a reason for hiding this comment

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

ReadonlyArray

Copy link
Author

Choose a reason for hiding this comment

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

Done

src/index.ts Outdated
}

searchUsersByFavoriteColor(color: string): Array<IUser>{
Copy link

Choose a reason for hiding this comment

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

ReadonlyArray

Copy link
Author

Choose a reason for hiding this comment

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

Done

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