style(linting): enable tsc's 'noUnusedParameters' compiler option

[tetsuharuohzeki]

Description:

• Make signatures for publicone, protected member, or exported function only suppress compile error.
  • They are opened interface. We cannot cause breaking change.
• Change (remove) needless signatures of private member or private functions.
  • They are not opened interface. This change will not breaking change.
• Relate Use new features introduced from TypeScript Compiler v2.0 #1837

Related issue (if exists):

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling d45e066 on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling d45e066 on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling d45e066 on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling d45e066 on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[tetsuharuohzeki]

umm, why is the coverage down?

[kwonoj]

I saw coverage confused time to time. By given numbers of coverage, I think this is same case.

[kwonoj]

Change itself looks ok to me.

[benlesh]

I'm not sure about this change.

1. What issue is it resolving? It's touching a LOT of files and I'm not sure what the gain is for the PR as a whole.
2. Sorting the compiler options, while helpful, will obfuscate changes visible in git blame on that config file. I understand the spirit, but I'm not sure about the result.

[benlesh]

Also, this should probably not be listed as a feat(TypeScript), because it's not really a feature change for the library is it? It seems like it's just a style(linting) sort of change?

[kwonoj]

It seems like it's just a style(linting) sort of change

Yes, I'd agree this is more about style changes to explicitly mark unused variables in function signatures, etcs. (Using TS@2 way)

[jayphelps]

My initial feeling is that a lot of these changes like

-    const result = source.retryWhen((errors: any) => notifier);
+    const result = source.retryWhen((_: any) => notifier);

Make the params even more frivolous, so wouldn't we just want to remove them instead of using the underscore unused convention? Thoughts?

[kwonoj]

so wouldn't we just want to remove them instead of using the underscore unused convention? Thoughts?

In my case I prefer to have param like (_value, key) => { do(key); } which shows signature supplies param, but it's not used inside of functions. My vote is let function have all param signatures (even it makes to disable noUnusedParameters) to have clarity regarding type definitions.

[benlesh]

In my case I prefer to have param like (_value, key) => { do(key); } which shows signature supplies param, but it's not used inside of functions

Linting shouldn't complain about that anyhow.

[jayphelps]

@kwonoj yeah I was referring to all the numerous places where we don't use any of the the params at all. In your (_value, key) => { do(key); } example, any reasonable linter shouldn't complain, no matter what you name your first arg, because you use the second one. Dunno if TS is a reasonable linter haha but I imagine it is.

[kwonoj]

@jayphelps yes, tslint and typescript compiler both should work with above snippet example, I have code working with ts@2 (https://github.com/kwonoj/rxjs-testscheduler-compat/blob/master/src/VirtualPromise.ts#L80)

[tetsuharuohzeki]

@jayphelps

Make the params even more frivolous, so wouldn't we just want to remove them instead of using the underscore unused convention? Thoughts?

As my thought of when I created this patch, it is just same with @kwonoj 's comment:

My vote is let function have all param signatures (even it makes to disable noUnusedParameters) to have clarity regarding type definitions.

I prefer to keep their signatures explicitly, and this patch was created with simply grep+replace.

[tetsuharuohzeki]

@Blesh

What issue is it resolving? It's touching a LOT of files and I'm not sure what the gain is for the PR as a whole.

This marks unused parameters explicitly. This will notify us that there are some cleanup chance which may be related to large refactoring. And we can suppress the error with adding _ prefix like this patch. I don't think this causes a trouble for our coding.

---

Sorting the compiler options, while helpful, will obfuscate changes visible in git blame on that config file. I understand the spirit, but I'm not sure about the result.

Okay. I'll remove its patch.

---

Also, this should probably not be listed as a feat(TypeScript), because it's not really a feature change for the library is it? It seems like it's just a style(linting) sort of change?

Okay. I'll change commit logs.

[kwonoj]

Let me try to summarize to get some clarification, as well as to get agreement for this changes.

1. current
(value: any, key: any) => { do(key); }

2. mark unused, leave param detail as-is
(_value: any, key: any) => { do(key); }

3. proposal in this pr, removes param detail
(_: any, key: any) => { do(key); }

4. remove param from signature
(key: any) => { do(key); }

my vote is 2, if not available falls back to 1 (even if it does not allow to enable compiler option).

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling ec36e3d on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[tetsuharuohzeki]

@kwonoj

2.mark unused, leave param detail as-is

(_value: any, key: any) => { do(key); }

Ah, nice. I'll change to sort the style with 2.

3.proposal in this pr, removes param detail

(_: any, key: any) => { do(key); }

I may use this style in some case in other language, but in TypeScript in this project, I don't have strong opinion to use style 3.

[tetsuharuohzeki]

-  notifyComplete(unused: Subscriber<R>): void {
+  notifyComplete(_: Subscriber<R>): void {

...But this case, I'm favor of to use _ to express "unused". It's a self-evident.

[tetsuharuohzeki]

...But this case, I'm favor of to use _ to express "unused". It's a self-evident.

Sorry. This is my wrong. They should use the original interface's argument name.

[jayphelps]

I think we should leave everything as-is and leave it up to the discretion of the person writing a given block of code. I have different opinions for apps, but for libs like this, I think we can't easily apply the rule across the entire codebase. Sometimes it makes sense to have the unused params because they will show up in IDE suggestion signatures to consumers of RxJS--and in that case they should not have _underscore _name because that implies they are private (by JS convention). Other times, in completely internal code, it make make sense to leave the params named for future documentation cases. They may be unused right now, but when I come back it's nice to see them and gives me immediate context around what this function is--especially when they're anonymous functions. And certainly there are times where it's very obvious what they would be and having them at all is potentially confusing.

So this all leads me to believe "it depends". So I think it's OK to just leave it to discretion.

Obviously, I'm always open to counterarguments 💃

[tetsuharuohzeki]

@jayphelps

and in that case they should not have _underscore _name because that implies they are private (by JS convention)
Other times, in completely internal code, it make make sense to leave the params named for future documentation cases.
#2024 (comment)

Yes. I agree to it completely. We should not use _ prefix for public interface.
So I don't change a public signature (e.g. Observable.map() or other public instance method of key fundamental components) in pull request, and I would not like to do it.

I also think that MergeMapSubscriber or other internal classes are exported to public and user can import to their codes, but they are private classes factually. Their classes are undocumented, I don't think many many user would use them as tier 1 public APIs. So I think we can change their classes signatures with _ prefix

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling 61f114d on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[tetsuharuohzeki]

I'll change to sort the style with 2.

done.

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling f7b7260 on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling 4d2e63d on saneyuki:compiler-config into 1bee98a on ReactiveX:master.

[benlesh]

I'm sorry, @saneyuki. I think a LOT of work and thought went into this PR, but I'm just not on-board with the changes.

1. It touches a lot of lines in a lot of files and doesn't fix any issues. Thus adding some indirection when using tools like git blame to figure out what's going on.
2. The outcome might be less ergonomic for other library authors.

I love the spirit and the hard work. I'd like to reward that sort of thing by merging it, however, I don't think I want to merge this particular PR, because I don't agree with the outcome.

In the future, it might be better to propose changes in an Issue first, and let the community discuss it before you put in all this hard work. I don't want any hard feelings, and I don't want to
waste anyone's time.

[tetsuharuohzeki]

I fixed needless changing line feed. Sorry.

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling 69c6184 on saneyuki:compiler-config into 4c31974 on ReactiveX:master.

[tetsuharuohzeki]

@Blesh

It touches a lot of lines in a lot of files and doesn't fix any issues. Thus adding some indirection when using tools like git blame to figure out what's going on.

If you said about my patch was changing some files' linefeed, I'm sorry my faults. I have been fixed them.

git blame is important, but we have a complete history in our repository and we're using GitHub that provides pretty nice UI to track a file not only git blame. I don't think that it would not be a serious problem about git blame.

I think we should build a more solid codes rather than clean git blame, and I hope it is valuable.

---

The outcome might be less ergonomic for other library authors.

I just think this noUnusedParameters compiler option is just a builtin rules which is like tslint's no-unused-variable error or others. We can escape from it easily with adding _ prefix. There is no problem. If we says about less ergonomic with lint errors, we should stop to use tslint to build a solid code.

And we enables tslint's no-unused-variable rules. This pull request do a same things on compiler's built feature.

---

In the future, it might be better to propose changes in an Issue first, and let the community discuss it before you put in all this hard work. I don't want any hard feelings, and I don't want to
waste anyone's time.

I'm apologize that I missed to note about the issue #1837. at the time to open this. But there is no any objection, I created this pull request.

[jayphelps]

@saneyuki Going to close for now and we may revisit later as the project progresses. Like @Blesh said, we do truly appreciate the effort that went into this. We want you to continue to feel welcome to contribute! Regarding #1837, we did see it, but it included many suggestions and at the time we hadn't decided when we were going to upgrade to tsc 2.0 so we didn't give any input because those things are just lower on our radar--we're pretty heads down focusing on trying to get 1.0-final shipped ASAP. So tl;dr no objection isn't necessarily a consensus of approval.

We're going to be meeting much more frequently to review outstanding PRs and issues, with the intent to get better at preventing these sorts of things. Bare with us as we learn and grow together! 👍

[tetsuharuohzeki]

@jayphelps

Regarding #1837, we did see it, but it included many suggestions and at the time we hadn't decided when we were going to upgrade to tsc 2.0 so we didn't give any input because those things are just lower on our radar

As the history, I had filed each of their new features as separated issues (#1969, #1972, #1971, #1970), but I gathered them into #1837 because @kwonoj says they should be gathered into the single issue.

If we need more discussion at an issue, not at here, I’ll reopen their separate issues.

---

we're pretty heads down focusing on trying to get 1.0-final shipped ASAP.

I agree that this issue would not be a release blocker to ship final 1.0-stable release. I agree we don't merge this and we don't merge or review this until shipping 1.0-final stable release too.

But it is not a reason to reject this pull request and to comment "file an issue at first" which is not listed in https://github.com/ReactiveX/rxjs/blob/master/CONTRIBUTING.md. AT NOW, there is no rules about "file an issue before opening pull request". Should we note about “file an issue at first before opening a new pull request” to a contribution guide?

And so I don't close this pull request by my hand without to explain why we would not like to keep open this.

---

So tl;dr no objection isn't necessarily a consensus of approval.

Do you mean "we must hear from all humans living on the Earth"? If you say so, we should build a new proposal process. AT NOW, we don't have concrete proposal process. I thought as a community contributor regards that there is no concrete process and we use implicit devolution if someone don't say any opinions. If my thought was wrong, please write your thought about proposal consensus process to a opened site.

---

We're going to be meeting much more frequently to review outstanding PRs and issues, with the intent to get better at preventing these sorts of things. Bare with us as we learn and grow together!

Yes. I'd also like to continue to effort to grow this project together. To achieve it, I’m going to discuss deeply.

Will you publish a public meeting note which are not published now? Openness very very important for contributors who would like to contribute a code. Private chatting & no publishing meeting notes are strong barrier for contributors. Could you publish them? I’m looking forward to see them!

[jayphelps]

@saneyuki we do indeed plan to start publishing our meeting notes. We're working out where to put them and who'll record them. 👍

[coveralls]

Coverage decreased (-0.0009%) to 97.038% when pulling 69c6184 on saneyuki:compiler-config into 4c31974 on ReactiveX:master.

[jayphelps]

@saneyuki Hey, we discussed this PR at our meeting today. (meeting notes)

We couldn't reach consensus on whether we want to impose this linting rule with some in favor and others against. Because we're trying to focus on shipping v5, we decided to keep the existing behavior for now.

We really do appreciate you taking the time! Having PRs closed can suck sometimes, but it does happen to all of us. We're also learning from this and going to discuss whether or not we should formalize a RFC process or at least document our suggestions better. 👍

[tetsuharuohzeki]

@jayphelps

We couldn't reach consensus on whether we want to impose this linting rule with some in favor and others against. Because we're trying to focus on shipping v5, we decided to keep the existing behavior for now.

Okay. I understand why we close this now.
I'll re-open #1970 as the tracking to noUnusedParameters.

[lock]

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.