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

Remove check of source files done in lint hook #247

Merged
merged 2 commits into from
May 15, 2020

Conversation

Blast545
Copy link
Contributor

As the title says, this will close #237. The comment was reworded as well to "source files" as this distinction is made on the ament_copyright linter.

Signed-off-by: Jorge Perez jjperez@ekumenlabs.com

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@Blast545 would it make sense to add LICENSE and CONTRIBUTING.md to the glob list instead of removing it entirely?

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

Blast545 commented May 15, 2020

@hidmic That would imply the linter will skip checking a package if it doesn't have a LICENSE/CONTRIBUTING.md file and I think it's desirable to report the failure when those files are not available.

As an added benefit, this will allow having the definition of "source files" only in the linter.

@Blast545
Copy link
Contributor Author

This will cause build problems with spdlog_vendor and console_bridge_vendor packages that added the ament_copyright linter an extra time. I will remove these extra additions before merging this.

Reference failing build for spdlog_vendor

  • Linux Build Status

@Blast545
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Blast545 Blast545 requested a review from hidmic May 15, 2020 19:20
@Blast545 Blast545 merged commit 038c9df into master May 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the blast545/remove_extra_check branch May 15, 2020 19:50
@dirk-thomas
Copy link
Contributor

It would have been good to mention "copyright" in the title since that is being used in the squashed commit message.

@dirk-thomas
Copy link
Contributor

@Blast545 It seems this change is responsible for multiple new copyright test failures in the nighty jobs. Please fix asap.

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.

ament_lint_common skipping ament_copyright when no source files
4 participants