Skip to content

Fix 10438 - Remove parameters of private methods and constructors in declaration files #11030

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

Closed
wants to merge 3 commits into from

Conversation

Igorbek
Copy link
Contributor

@Igorbek Igorbek commented Sep 21, 2016

Fixes #10438

This PR makes these changes in declaration emitting:

  • private instance methods to be emitted as private methodName;
  • private static methods to be emitted as private static methodName;
  • private constructor to be emitted as private constructor();
  • if private method or constructor has overloads then only single declaration is emitted
  • instance and static members with the same name are not considered overloads

Open questions:

  • is it ok to emit private constructors with removed parameters?
    • I think it is ok, since nobody would be able to call it anyway

#myfirsttypescriptpr

@msftclas
Copy link

Hi @Igorbek, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

@Igorbek, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@Igorbek
Copy link
Contributor Author

Igorbek commented Oct 12, 2016

Is it going to be reviewed/merged?

@@ -1352,6 +1360,15 @@ namespace ts {
// If we are emitting Method/Constructor it isn't moduleElement and hence already determined to be emitting
// so no need to verify if the declaration is visible
if (!resolver.isImplementationOfOverload(node)) {
// If we are emitting a private method or constructor it might be already emitted with an overloaded signature
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect way of handling it.
You could just check in emitSignatureDeclaration if this is first signature and skip rest for private methods.
Map of name is not needed and if needed you can ask resolver for the question instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I'll change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, writeFunctionDeclaration first writes declaration flags and function name and then calls emitSignatureDeclaration. So it seems here's the proper place for checking.
Do you suggest to add a new question to resolver? Because I didn't find any that matches that need.

Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

Need change in approach with method declaration emit

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2017

Thanks for your contribution. This PR has failing CI tests and can not be merged in at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the master branch.

@mhegazy mhegazy closed this May 22, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants