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.

Sign up for GitHub

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

Jump to bottom

Ensure file, include and exclude specs used are strings #40041

Merged
merged 3 commits into from Aug 14, 2020
Merged

Ensure file, include and exclude specs used are strings #40041

merged 3 commits into from Aug 14, 2020

Conversation

sheetalkamat
Copy link
Member

Fixes #38164, #39856

@DanielRosenwasser
Copy link
Member

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 13, 2020 •
edited

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 959dee4. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 13, 2020 •
edited

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 959dee4. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 13, 2020 •
edited

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 959dee4. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

@typescript cherry-pick this to release-4.0 and LKG

@typescript-bot typescript-bot mentioned this pull request Aug 13, 2020
Open
@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

sandersn
sandersn requested changes Aug 13, 2020
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

The fix looks right, but the return type is awfully hard to read. I have a straw-man suggestion for making it explicit.

src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
sandersn
sandersn approved these changes Aug 13, 2020
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good; one question about what to return when array elements have the wrong type.

src/compiler/commandLineParser.ts Show resolved Hide resolved
@sheetalkamat
Copy link
Member Author

@typescript cherry-pick this to release-4.0 and LKG

@sheetalkamat sheetalkamat merged commit c95cffe into master Aug 14, 2020
@sheetalkamat sheetalkamat deleted the nonStringSpecs branch August 14, 2020 00:09
@sheetalkamat
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.0 and LKG

@DanielRosenwasser DanielRosenwasser restored the nonStringSpecs branch August 14, 2020 23:58
@DanielRosenwasser DanielRosenwasser deleted the nonStringSpecs branch August 14, 2020 23:58
DanielRosenwasser pushed a commit that referenced this pull request Aug 14, 2020
@sheetalkamat @DanielRosenwasser
Ensure file, include and exclude specs used are strings (#40041)
1030455 
* Test displaying failure when specs used are not strings

* Ensure specs used are strings
Fixes #38164, #39856

* Feedback
@j-oliveras
Copy link
Contributor

@sheetalkamat #39856 is still open but should be fixed by this PR.

@sheetalkamat
Copy link
Member Author

Thanks closed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

@weswigham weswigham Awaiting requested review from weswigham

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

Assignees

@sheetalkamat sheetalkamat

Labels
Author: Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TS Server fatal error: path.replace is not a function
5 participants
@sheetalkamat @DanielRosenwasser @typescript-bot @j-oliveras @sandersn