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

[ci] add check for copyright header #41

Merged

Conversation

tijyojwad
Copy link
Contributor

Add script to check for copyright header
Fail CI if copyright headers are missing

@tijyojwad tijyojwad self-assigned this Jul 18, 2019
@tijyojwad tijyojwad force-pushed the jdaw/copyright-check branch 4 times, most recently from d94767a to 4f45d5d Compare July 18, 2019 19:13
@tijyojwad tijyojwad requested a review from vellamike July 18, 2019 19:31
@tijyojwad tijyojwad assigned vellamike and unassigned tijyojwad Jul 18, 2019
@@ -0,0 +1,169 @@
#!/usr/bin/python
Copy link
Contributor

Choose a reason for hiding this comment

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

/usr/bin/env python3


cpp_exts = {".hpp", ".cpp", ".cu", ".cuh", ".cc", ".h", ".c"}

cpp_copyright = """/*
Copy link
Contributor

Choose a reason for hiding this comment

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

probably best put an r in front of this string

ci/checks/check_copyright.py Show resolved Hide resolved
Args:
f - Path to file
"""
if 'Copyright (c)' in open(f).read():
Copy link
Contributor

Choose a reason for hiding this comment

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

return 'Copyright (c)' in open(f).read() more pythonic

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, don't know if you want to just use the regex module to check if the string we have is in there, might make it easier also to replace existing copyright notices etc


# Get list of files to check.
script_dir = os.path.dirname(os.path.realpath(__file__))
root_dir = os.path.abspath(os.path.join(script_dir, "../../"))
Copy link
Contributor

Choose a reason for hiding this comment

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

better to use pathlib.

from pathlib import Path
Path('foo').parent

.. will break OS interop


# Check/Add headers if need be.
if (args.add_header):
map(add_copyright, missing_header_files)
Copy link
Contributor

Choose a reason for hiding this comment

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

a matter of style, but list comprehensions are preferred to map


import sys
import os
import mmap
Copy link
Contributor

Choose a reason for hiding this comment

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

unused import

import argparse
import re
from subprocess import check_output
from stat import *
Copy link
Contributor

Choose a reason for hiding this comment

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

best to never use import *

add optionally copyright header to them.
"""

import sys
Copy link
Contributor

Choose a reason for hiding this comment

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

could these imports be sorted alphabetically

script_dir = os.path.dirname(os.path.realpath(__file__))
root_dir = os.path.abspath(os.path.join(script_dir, "../../"))
files = get_tracked_files(root_dir)
files = filter_files(files, "LICENSE|README|data\/|docs\/")
Copy link
Contributor

@vellamike vellamike Jul 19, 2019

Choose a reason for hiding this comment

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

this string should be a raw string r"LICENSE|README|data\/|docs\/". Otherwise the \/ character combination can be interpreted as an escape sequence (probably an invalid one) by different tools, editors etc

@tijyojwad tijyojwad force-pushed the jdaw/copyright-check branch 4 times, most recently from a971b39 to 6060670 Compare July 19, 2019 16:55
@tijyojwad tijyojwad assigned tijyojwad and unassigned vellamike Jul 19, 2019
@tijyojwad tijyojwad assigned vellamike and unassigned tijyojwad Jul 19, 2019
Joyjit Daw added 2 commits July 19, 2019 13:16
Add script to check for copyright header
Fail CI if copyright headers are missing
@vellamike vellamike merged commit f688bb9 into NVIDIA-Genomics-Research:dev-v0.2.0 Jul 21, 2019
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.

None yet

2 participants