Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

export-name: detect export declarations #443

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

amacleay
Copy link
Contributor

@amacleay amacleay commented Jun 20, 2018

Previously, the export-name rule would detect an export assignment
like

export = Fish;
// or
export default Fish;

but it would not detect an export like

export { Fish }

Fixes #444

Previously, the `export-name` rule would detect an export assignment
like

```
export = Fish;
// or
export default Fish;
```

but it would not detect an export like

```
export { Fish }
```
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Other than a missing test case, LGTM!

@@ -315,6 +315,24 @@ describe('exportNameRule', () : void => {
]);
});

it('when mis-named function is exported in a separate statement', () : void => {

Choose a reason for hiding this comment

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

Good test for the case of complaining. There should also be a corresponding test for the case of the user getting it right, e.g. file.ts having export { file };.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in new commit, good catch!

@@ -46,26 +46,43 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

function isExportedDeclaration(element: ts.Statement): boolean {

Choose a reason for hiding this comment

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

Just a suggestion: you may wish to file issues on https://github.com/ajafff/tsutils to add these there, as they seem useful. Not a blocking issue for this pull request at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it: ajafff/tsutils#68 - feel free to comment there if it's not clear or not quite what you expected.

@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! label Jul 5, 2018
@JoshuaKGoldberg JoshuaKGoldberg added Type: Breaking Change and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Jul 5, 2018
@JoshuaKGoldberg JoshuaKGoldberg merged commit 1c14cd2 into microsoft:master Jul 5, 2018
@JoshuaKGoldberg
Copy link

Awesome.

Technically, this is a breaking change, as it should start flagging violations it wasn't before.

@JoshuaKGoldberg JoshuaKGoldberg added this to the 5.0.4 milestone Jul 18, 2018
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.

2 participants