Skip to content

Type the result property on IDBRequest #530

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

Merged
merged 7 commits into from
Jul 12, 2018
Merged

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Jul 10, 2018

src/emitter.ts Outdated
if (!typeParameters) return i.name;
if (!typeParameters.length) return i.name;

return `${i.name}<${typeParameters.map(t => t.replace(/\s+=.*$/, ''))}>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would rather we have two separate properties, one for the name of the type parameter, and one for the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy what do you think about this?

"type-parameters"?: { name: string, default?: string }[]

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 10, 2018

//CC @saschanaz

@LinusU
Copy link
Contributor Author

LinusU commented Jul 11, 2018

  • ✅ Rebased on master
  • ✅ Added printing of a diff when generated doesn't match the baseline
  • ✅ Updated the normalizeLineEndings function to normalize the trailing line end as well
  • ✅ Separated type parameters into a name and default property

src/test.ts Outdated

const __SOURCE_DIRECTORY__ = __dirname;
const baselineFolder = path.join(__SOURCE_DIRECTORY__, "../", "baselines");
const outputFolder = path.join(__SOURCE_DIRECTORY__, "../", "generated");
const tscPath = path.join(__SOURCE_DIRECTORY__, "../", "node_modules", "typescript", "lib", "tsc.js");

function normalizeLineEndings(text: string): string {
return text.replace(/\r\n?/g, "\n");
return text.replace(/\r\n?/g, "\n").replace(/\n*$/, "\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this is needed, does \n\n$ thing happen?

Copy link
Contributor Author

@LinusU LinusU Jul 11, 2018

Choose a reason for hiding this comment

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

The files in git doesn’t have a trailing newline at all, but my editor (VS Code) adds one when I open the file. Thus I always got a diff.

This can of course be turned off in my editor, but I thought it was common enough so that it would help to ignore it. I think Atom is also adding trailing newline by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK VSCode adds a newline when you save the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry, saved is correct. The workflow I've been using is to first modify the baseline files to be what I want, and then work with the json files to get that result. That's when I ran into the problem, since I had saved a new baseline file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Um... You probably will want to try npm run build && npm run baseline-accept 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, yeah, although I wasn't familiar with how the JSON files worked. So it was actually a quite nice workflow for me, since I could set up my target and then work towards that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally still don't like this, how about adding a trailing newline through emitter.ts rather than ignoring it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me, I was actually close to doing that instead, as it's actually required for the Unix definition of a text file 😄 ref

Pushing a fix now 🙌

@mhegazy
Copy link
Contributor

mhegazy commented Jul 11, 2018

@saschanaz can i get a sign off?

@LinusU
Copy link
Contributor Author

LinusU commented Jul 12, 2018

  • ✅ Removed the normalization of newlines in test
  • ✅ Emit newline at the end of the generated files

@mhegazy mhegazy merged commit bbf761b into microsoft:master Jul 12, 2018
@LinusU LinusU deleted the idbrequest branch July 13, 2018 06:04
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.

3 participants