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

style(linting): enable ' --noUnusedLocals' option #2104

Closed
wants to merge 2 commits into from
Closed

style(linting): enable ' --noUnusedLocals' option #2104

wants to merge 2 commits into from

Conversation

tetsuharuohzeki
Copy link
Contributor

@tetsuharuohzeki tetsuharuohzeki commented Nov 4, 2016 •
edited
Loading

Description:

  • This option detects more unused variables than tslint.
  • We detect unused variables at compile time without doing lint.
  • We can replace tslint's no-unused-variable (But not now).
    • We need to separate tslint configurations for src/ from single file.
  • This pull request does not change a public interface.
    • Almost changes are related to private interface.

Related issue (if exists):

@tetsuharuohzeki
refactor(ErrorObservable): clarify an error type E for ErrorObserva…
b5fb259 
…ble.

BREAKING CHANGE - `ErrorObservable.create()` (`Obserbable.throw()`)
takes a new type parameter `E` that express a error type instead on old
one's `T`. Their functions return `ErrorObservable<E>`.
It is still the drived type of `Observable<any>`, it's not changed from
old one. But it contains the error typed as `E` now.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 97.596% when pulling 4a0ec5c on saneyuki:noUnusedLocals into 39214f2 on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 97.596% when pulling 4a0ec5c on saneyuki:noUnusedLocals into 39214f2 on ReactiveX:master.

jayphelps
jayphelps reviewed Nov 4, 2016
View reviewed changes
src/observable/ArrayLikeObservable.ts
@@ -55,7 +55,8 @@ export class ArrayLikeObservable<T> extends Observable<T> {

protected _subscribe(subscriber: Subscriber<T>): TeardownLogic {
let index = 0;
const { arrayLike, scheduler } = this;
const arrayLike = this.arrayLike;
const scheduler = this.scheduler;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason? I think we want to keep our existing, destructure style. Is there a subtle difference I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for to avoid a TypeScript compiler's bug (It will be fixed in TS2.1).

jayphelps
jayphelps reviewed Nov 4, 2016
View reviewed changes
src/operator/delay.ts
constructor(private time: number,
private notification: any) {
constructor(public time: number,
public notification: Notification<T>) {
Copy link
Member

Choose a reason for hiding this comment

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

Did these need to change from private to public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old code was wrong. These members should be public to be used in DelaySubscriber

jayphelps
jayphelps reviewed Nov 4, 2016
View reviewed changes
src/operator/last.ts
@@ -58,7 +58,7 @@ class LastSubscriber<T, R> extends Subscriber<T> {
constructor(destination: Subscriber<R>,
private predicate?: (value: T, index: number, source: Observable<T>) => boolean,
private resultSelector?: ((value: T, index: number) => R) | void,
private defaultValue?: any,
defaultValue?: any,
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to be changed to not be a private property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultValue is not used in LastSubscriber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this option make it the error.

jayphelps
jayphelps reviewed Nov 4, 2016
View reviewed changes
src/Rx.ts
queue: QueueScheduler;
animationFrame: AnimationFrameScheduler;
async: AsyncScheduler;
} = {
Copy link
Member

Choose a reason for hiding this comment

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

Are these annotations necessary? I figured TS could easily infer the types from the assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't make it, TypeScript compiler make it error (This might be a compiler's issue?). But there is no problem. We can avoid such error with just writing expliciltly code.

@jayphelps jayphelps mentioned this pull request Nov 4, 2016
Merged
@tetsuharuohzeki tetsuharuohzeki changed the title feat(TypeScript): enable ' --noUnusedLocals' option style(linting): enable ' --noUnusedLocals' option Nov 7, 2016
@jayphelps
Copy link
Member

Discussed this one at our last meeting and we decided we want to keep the existing behavior because we haven't noticed any practical issues with our current way and this makes debugging harder--leaving unused variables while we're fixing something, but being reminded if we forget by the separate linting phase before commit.

Thanks @saneyuki for your contribution!

More info: https://github.com/ReactiveX/rxjs-core-notes/blob/master/2016-11/november-8.md#internal-lint-changes-2104-2103

@jayphelps jayphelps closed this Nov 9, 2016
@tetsuharuohzeki tetsuharuohzeki deleted the noUnusedLocals branch November 10, 2016 04:59
@tetsuharuohzeki
Copy link
Contributor Author

@jayphelps

Okay. I accept this decision.

I might open a new pull request which remove some needless code detected by noUnusedLocals option but not enable it.

tetsuharuohzeki added a commit to tetsuharuohzeki/rxjs that referenced this pull request Nov 10, 2016
@tetsuharuohzeki
style(linting): clarify the type of 2nd argument of Operator<T, R>.ca…
62d8933 
…ll()

- This commit is spin-off of ReactiveX#2104
@tetsuharuohzeki tetsuharuohzeki mentioned this pull request Nov 10, 2016
Closed
tetsuharuohzeki added a commit to tetsuharuohzeki/rxjs that referenced this pull request Nov 15, 2016
@tetsuharuohzeki
style(linting): clarify the type of 2nd argument of Operator<T, R>.ca…
974ffe8 
…ll()

- This commit is spin-off of ReactiveX#2104
@kwonoj kwonoj mentioned this pull request Nov 19, 2016
Closed
tetsuharuohzeki added a commit to tetsuharuohzeki/rxjs that referenced this pull request Nov 30, 2016
@tetsuharuohzeki
style(linting): clarify the type of 2nd argument of Operator<T, R>.ca…
9cfc542 
…ll()

- This commit is spin-off of ReactiveX#2104
tetsuharuohzeki added a commit to tetsuharuohzeki/rxjs that referenced this pull request Dec 13, 2016
@tetsuharuohzeki
style(linting): clarify the type of 2nd argument of Operator<T, R>.ca…
22f7e48 
…ll()

- This commit is spin-off of ReactiveX#2104
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jayphelps jayphelps jayphelps left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tetsuharuohzeki @coveralls @jayphelps