Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

Update "any" types to more precise #230

Closed
wants to merge 3 commits into from

Conversation

GrayStrider
Copy link
Contributor

@GrayStrider GrayStrider commented Sep 30, 2019

Consolidate repeating types, augment express.Request with User model (copies #227 )
Update User model:

user[provider] = undefined;
seem to be unused, otherwise need to introduce indexing field to UserDocument

Update User model
update "any" types to more precise
@msftclas
Copy link

msftclas commented Sep 30, 2019

CLA assistant check
All CLA requirements met.

@GrayStrider
Copy link
Contributor Author

Just figuring out how everything works, sorry if it's rough around the edges
Oh wow, build passed! yay 🎉

src/controllers/user.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@peterblazejewicz peterblazejewicz left a comment

Choose a reason for hiding this comment

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

Please remove commented code like:

// [provider: string]: any;

(either uncomment if temporary changes or remove completetly , or update comment with specific information

Remove direct use of genericExpressMethod, using Request/Response/NextFunction should work

src/types/express-request-with-user.d.ts Outdated Show resolved Hide resolved
src/config/passport.ts Outdated Show resolved Hide resolved
src/config/passport.ts Outdated Show resolved Hide resolved
src/controllers/api.ts Outdated Show resolved Hide resolved
src/controllers/user.ts Outdated Show resolved Hide resolved
no-explicit-any: warning,
no-inferrable-types: warning except for parameters

update (remove) "any" types
@GrayStrider
Copy link
Contributor Author

GrayStrider commented Oct 1, 2019

Can a ask for a bit of advice, please: what if I want to push to my fork branch, but don't want to include these commits to this pull request? Or I should just wait for this PR to be closed and in the meantime use a different branch?
I'll revert the last commit if needed, and perhaps submit it separetely, but it's kinda based on the previous ones partially. What do?

@peterblazejewicz
Copy link
Collaborator

  • you could work on this branch as long as you want. It seems you have started directly from your local master branch. Usually people should create a topic branch to work with, but even in the GitHub docs using master fork branch is being used:
    https://github.com/collections/choosing-projects
    Your changes can be later squashed into a single commit if accepted, so usually that does not matter how many changes you push to your PR

  • you could create new local topic branch on work on the other changes as you want and use that new branch as base for your PR against this repo (this doc does not use master, it explain why to not user master in forks):
    https://help.github.com/en/articles/creating-a-pull-request
    Using new topic branch is preferable for variety of different reasons.

Up to you, you've even used very new draft feature to make this PR:
https://github.blog/2019-02-14-introducing-draft-pull-requests/

@GrayStrider
Copy link
Contributor Author

created new PR from separate branch #232

@GrayStrider GrayStrider closed this Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants