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

Added summary on warnings and errors (#2581) #2583

Merged
merged 8 commits into from
Jun 11, 2024

Conversation

bladerunner2020
Copy link
Contributor

@bladerunner2020 bladerunner2020 commented Jun 1, 2024

Added a summary message about warnings and errors if any (see #2581) in the following functions:

  • generateDocs
  • generateJson

Added tests.

Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a couple concerns

const { errorCount, warningCount } = this.logger;
return `Found: ${errorCount} error${
errorCount === 1 ? "" : "s"
}, ${warningCount} warning${warningCount === 1 ? "" : "s"}.`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Internationalizing this is going to be a massive pain... in TypeDoc 0.26 this needs to be a templated string with a placeholder. I think I'd rather just keep this simple here to make that migration easier:

return `Found ${errorCount} error(s), ${warningCount} warning(s).`

@@ -525,6 +525,14 @@ export class Application extends ChildableComponent<
const start = Date.now();
out = Path.resolve(out);
await this.renderer.render(project, out);

// Generate a message summarizing the number of errors and warnings if there are any
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the wrong place to put this. TypeDoc has a --watch option, which will result in generateDocs and generateJson being called many times. If a user has both options turned on, then with this implementation they may get two warnings/errors for each compilation.

I think the way I'd prefer to handle this is to add a public logRunSummary (better name?) method on Application which is called in cli.ts in the appropriate places

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also has the benefit of making the tests for this feature much less hacky - I've very intentionally avoided mocking pieces of TypeDoc in tests (almost) everywhere so far, they tend to lead to testing implementation details+easily broken tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the wrong place to put this. TypeDoc has a --watch option, which will result in generateDocs and generateJson being called many times. If a user has both options turned on, then with this implementation they may get two warnings/errors for each compilation.

I think the way I'd prefer to handle this is to add a public logRunSummary (better name?) method on Application which is called in cli.ts in the appropriate places

That seems reasonable. I'll take a look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at the code again, and now I have some doubts. I think the best place for logging a summary is just before the line "Documentation generated at...". If I were to run TypeDoc in watch mode, I don't think I would mind seeing it every time I changed a file. On the contrary, it would be quite handy in some cases, for example, when I am working on reducing the number of warnings.

That said, do you still want me to move the summary? 😊

@bladerunner2020 bladerunner2020 requested a review from Gerrit0 June 11, 2024 19:20
@bladerunner2020
Copy link
Contributor Author

I moved logRunSummary to cli.ts and agree that it's a better place for it. I also removed the test file as the tests are no longer necessary due to the simplicity of this solution. However, I did not add logRunSummary to the Application; I left it as a function in cli.ts.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 11, 2024

Thanks!

@Gerrit0 Gerrit0 merged commit 2baf578 into TypeStrong:master Jun 11, 2024
4 checks passed
@Gerrit0 Gerrit0 added this to the v0.26.0 milestone Jun 11, 2024
Gerrit0 pushed a commit that referenced this pull request Jun 12, 2024
* feat: added a message summarizing the number of errors and warnings if there are any, fix: VERSION property is readonly

* tests: added test for errors and warnings summary message

* tests: added test for errors and warnings summary message (generateJson)

* chore: added a comment

* fix: typescript errors

* fix: fixed warnings by prettier

* refactor: moved logRunSummary to cli

* fix: lint issues
@Dayday10
Copy link

Dayday10 commented Jun 12, 2024 via email

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.

3 participants