Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Require semicolon for type alias #1475

Merged
merged 1 commit into from Aug 11, 2016
Merged

Require semicolon for type alias #1475

merged 1 commit into from Aug 11, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 11, 2016

This would require all type t = foo; to end with a semicolon.

@adidahiya
Copy link
Contributor

👍 thanks @andy-ms!

@adidahiya adidahiya added this to the TSLint v3.15 milestone Aug 11, 2016
@adidahiya adidahiya merged commit 6f3d73b into palantir:master Aug 11, 2016
@jkillian
Copy link
Contributor

Hmm, this also is a breaking change, we should either (temporarily) revert this, or just start merging other breaking changes for 4.0

@ScottSWu
Copy link
Contributor

Oops, looking at the log, it looks like the new tests are just missing in the test.ts.fix files added with suggesting fixes.

@ghost
Copy link
Author

ghost commented Aug 11, 2016

I noticed that the .fix files have CRLF line endings while the .lint files have LF line endings, at least on my checkout. I was able to get tests working by adding results.fixesFromMarkup = results.fixesFromMarkup.replace(/\r\n/g, "\n"); to consoleTestResultHandler in test.ts (and by adding the missing type t = number; lines), but there is likely a better solution.

@ScottSWu
Copy link
Contributor

Interesting, it looks like diff.diffLines does consider both line endings, but only ignores them in the diff if {newlineIsToken: true} is specified. That argument isn't in the typings file, but is implemented.

@jkillian
Copy link
Contributor

Hmm, I'm confused why the Circle build didn't break as well, considering the .fix files didn't change?

Looks like the diff options you mentioned are documented though, so I'm happy to use them in our code: https://github.com/kpdecker/jsdiff#api

@jkillian
Copy link
Contributor

Going to revert this PR temporarily for now (#1477), but we'll get it back in for 4.0

@ghost
Copy link
Author

ghost commented Aug 12, 2016

Should I make a second PR with test fixes?

@jkillian
Copy link
Contributor

I'm not sure if I can "re-open" a merged PR on GH, so a new one would be great!

@ghost ghost mentioned this pull request Aug 12, 2016
@jkillian
Copy link
Contributor

#1478 created which adjusts the appropriate .fix files. @ScottSWu - have any idea why the CircleCI build passed on this PR in the first place?

@ghost
Copy link
Author

ghost commented Aug 12, 2016

The branch I had didn't have the .fix files on it at all, as it was out-of-date with the latest master. So a CI that just ran the contents of my branch (and not the potential merged contents) would succeed.

@jkillian
Copy link
Contributor

Ohh, I didn't realize it was off an out of date master, that makes perfect sense. I guess the underlying issue here then is to get CircleCI to run the tests on the result of the would-be-merge as well.

@jkillian
Copy link
Contributor

Seems as if CircleCI doesn't support this unfortunately.

@ghost ghost deleted the type_alias_semicolon branch August 24, 2016 18:17
soniro pushed a commit to soniro/tslint that referenced this pull request Aug 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants