Skip to content

Minor cleanup in bindNamespaceExportDeclaration #27367

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
2 commits merged into from
Oct 1, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 26, 2018

Broken out from #27281

  • There's a mix of if-else and also early returns. This makes the three cases look different, but they're actually all the same -- file a diagnostic and return.
  • They also all add the diagnostic in the same way and are distinguished only by the name. So we can put the diagnostic-adding code in one place.

@ghost ghost requested a review from sandersn September 26, 2018 18:56
file.bindDiagnostics.push(createDiagnosticForNode(node, Diagnostics.Global_module_exports_may_only_appear_at_top_level));
return;
const diag = !isSourceFile(node.parent)
? Diagnostics.Global_module_exports_may_only_appear_at_top_level
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a cleanup, can you instead use the syntax

preducate1 ? result1 :
predicate2 ? result2 :
undefined

I think that's a lot easier to read.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Formatting change request

@ghost
Copy link
Author

ghost commented Oct 1, 2018

@sandersn good to merge?

@ghost ghost merged commit 115636b into master Oct 1, 2018
@ghost ghost deleted the bindNamespaceExportDeclaration branch October 1, 2018 19:15
This pull request was closed.
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.

1 participant