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.

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

Warn the user if a comment looks like it was meant to be a module comment but isn't #1080

Closed
Gerrit0 opened this issue Jul 26, 2019 · 6 comments · Fixed by #1154
Closed

Warn the user if a comment looks like it was meant to be a module comment but isn't #1080

Gerrit0 opened this issue Jul 26, 2019 · 6 comments · Fixed by #1154
Assignees
Labels
enhancement Improved functionality

Comments

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jul 26, 2019

TypeDoc supports module level comments which document a file rather than a specific member of the file, but only if there are two doc comments before the first node in the file.

/** Module doc */

/** Function doc */
const noop = () => {}

This rule also applies to import statements, but users likely expect comments attached to them to be applied to the file. I don't think it makes sense to add (another) special case for module comments, but it would be nice to warn the user that they added a comment to an import declaration at the start of a file that looks like it might have been intended to be a module comment.

/** NOT a module doc */
import  'test'

const noop = () => {}

This warning should not appear when there is one comment before a node that is commonly documented. Right now I think the only nodes it should warn on are import statements.

@Gerrit0 Gerrit0 added enhancement Improved functionality help wanted Contributions are especially encouraged good first issue Easier issue for first time contributors labels Jul 26, 2019
@aciccarello
Copy link
Collaborator

Would this warn on all /** */ comments before imports?

@Gerrit0
Copy link
Collaborator Author

Gerrit0 commented Jul 26, 2019

Only if the /** */ comment was the first comment in the file. It shouldn't warn in this case:

import 'test'
/** doc */
import 'test2'

@aciccarello
Copy link
Collaborator

I like the idea of pointing out potential confusion, however I think that may be frustrating for people with file headers. That's part of why I've been hesitant to resolve the top of file comment questions. It's hard to know what they signify.

/**
 * @license
 * Copyright Google Inc. All Rights Reserved.
 *
 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.io/license
 */
import { x } from './core';
import { y } from './util';

That's not to say we can't come up with a solution but I want to be cautious about making a change that would require users to edit many of their files.

@Gerrit0
Copy link
Collaborator Author

Gerrit0 commented Jul 26, 2019

Good point about copyright blocks. I rather like the @packageDocumentation tag proposed in microsoft/tsdoc#6, but for backward compatibility we can't require it... this is more complicated than I thought at first.

@Gerrit0 Gerrit0 removed good first issue Easier issue for first time contributors help wanted Contributions are especially encouraged labels Jul 26, 2019
@Gerrit0
Copy link
Collaborator Author

Gerrit0 commented Jul 28, 2019

Coming back to this, supporting @packageDocumentation to flag a comment at the top specifically as package documentation seems reasonable to me. If there are 2+ comments we can still follow existing behavior, but if there is only one, and it is marked with @packageDocumentation it should be treated as such.

@aciccarello
Copy link
Collaborator

aciccarello commented Jul 29, 2019

👍 to supporting a @packageDocumentation tag. That's nice and explicit. We can support the existing 2+ comments as well so we can avoid having a breaking change but it would be good to indicate somehow that it is deprecated. We could note that in documentation or start emitting console warnings. We don't currently have a good deprecation process.

@Gerrit0 Gerrit0 self-assigned this Dec 26, 2019
Gerrit0 added a commit that referenced this issue Dec 26, 2019
Closes #1080

Also removes the grunt task, we mention the third-party grunt plugin on the website, and the built in one has been broken since 2015 without anyone complaining.
Gerrit0 added a commit that referenced this issue Dec 26, 2019
Closes #1080

Also removes the grunt task, we mention the third-party grunt plugin on the website, and the built in one has been broken since 2015 without anyone complaining.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improved functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants