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

add copyright lines to all source files #1996

Merged
merged 7 commits into from
Jun 24, 2022
Merged

add copyright lines to all source files #1996

merged 7 commits into from
Jun 24, 2022

Conversation

merobi-hub
Copy link
Collaborator

@merobi-hub merobi-hub commented May 19, 2022

Signed-off-by: Michael Robinson [email protected]

Problem

The project's source files lack a copyright, but inclusion of a copyright notice in every source file is an expectation of the LFAI.

Solution

This PR adds a copyright line to source files across the project (.py, .java, .sh), excluding non-source files and files where the line could create an issue.

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've updated the CHANGELOG.md with details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary)
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)

Copy link
Contributor

@mobuchowski mobuchowski left a comment

Choose a reason for hiding this comment

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

From the experience of doing the same in OpenLineage project - it would be better if this was a series of commits, each focused on particular file type only. This would allow spotting problems with particular file types.

For example, the copyright statement being top of file in .sh files is wrong - it makes shebang not work properly. This is not easily found when looking through 300+ files 😉

@merobi-hub
Copy link
Collaborator Author

Thanks, Maciej. I separated the changes into individual commits, one per file type.

@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #1996 (c9bf83f) into main (2783f4e) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1996   +/-   ##
=========================================
  Coverage     78.89%   78.89%           
  Complexity     1014     1014           
=========================================
  Files           197      197           
  Lines          5548     5548           
  Branches        421      421           
=========================================
  Hits           4377     4377           
  Misses          724      724           
  Partials        447      447           
Impacted Files Coverage Δ
api/src/main/java/marquez/MarquezApp.java 64.86% <ø> (ø)
api/src/main/java/marquez/MarquezConfig.java 100.00% <ø> (ø)
api/src/main/java/marquez/MarquezContext.java 84.93% <ø> (ø)
api/src/main/java/marquez/api/BaseResource.java 80.95% <ø> (ø)
api/src/main/java/marquez/api/DatasetResource.java 94.23% <ø> (ø)
api/src/main/java/marquez/api/JobResource.java 92.30% <ø> (ø)
...i/src/main/java/marquez/api/NamespaceResource.java 90.90% <ø> (ø)
...src/main/java/marquez/api/OpenLineageResource.java 84.21% <ø> (ø)
api/src/main/java/marquez/api/RunResource.java 86.66% <ø> (ø)
api/src/main/java/marquez/api/SearchResource.java 72.22% <ø> (ø)
... and 187 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@merobi-hub
Copy link
Collaborator Author

Hi @mobuchowski I made the requested change -- please review when you have a moment. Thanks.

Copy link
Member

@wslulciuc wslulciuc 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 adding the copyright to the source files, @merobi-hub! I did look over the files, and have the following suggestions:

  • Remove the copyright from all *.txt and *.md files
  • For *.sh files, the copyright has been added to the bottom of the scripts, and should be in the file header (I know this was done to address the comment by @mobuchowski, but ideally it should be in the header - see my suggestion below)
  • Is there a way to combining the copyright and license in a single header? That is:

java

/* 
 * Copyright 2018-2022 contributors to the Marquez project
 * SPDX-License-Identifier: Apache-2.0
 */

bash

   
#!/bin/bash
#
# Copyright 2018-2022 contributors to the Marquez project
# SPDX-License-Identifier: Apache-2.0

py

# Copyright 2018-2022 contributors to the Marquez project
# SPDX-License-Identifier: Apache-2.0

clients/python/marquez_client/__init__.py Outdated Show resolved Hide resolved
@merobi-hub
Copy link
Collaborator Author

merobi-hub commented Jun 8, 2022

Thanks for the feedback, @wslulciuc . Curious about why you want the line removed from .md and .txt files, assuming they're original to the project. Can you clarify?
Reformatting the lines would mean starting over and debugging again. Are we okay with extending the timeline for this?
Thanks.

@wslulciuc
Copy link
Member

wslulciuc commented Jun 8, 2022

Curious about why you want the line removed from .md and .txt files, assuming they're original to the project. Can you clarify?

The project MUST include a copyright statement in each source file

This is up for interpretation, but source files usually refer to code .java, .py, etc, and not docs. But ok I see, since the copyright for the *.md file is hidden using <!-- -->, you can leave it. Initially I had assumed it broke the formatting. For new docs, we'll want to add the copyright. Have you looked into adding the copyright to the files under docs/?

For the *.txt files, we can leave them but would address my most recent comment on the file web/src/fonts/Karla/OFL.txt

Reformatting the lines would mean starting over and debugging again. Are we okay with extending the timeline for this?

Normally, I won't care as much but this is such a rare and infrequent change that will be copied for all new source files that I'm ok with extending the timeline to use a more compact header at the top of the source files (like I outlined in my change request). I know, not fun and really sorry for the extra effort here. I owe you!

@merobi-hub
Copy link
Collaborator Author

Hi @mobuchowski and @wslulciuc , I think this is ready to be merged

Copy link
Member

@wslulciuc wslulciuc left a comment

Choose a reason for hiding this comment

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

Amazing work, @merobi-hub! 💯 🥇

web/src/fonts/Karla/OFL.txt Outdated Show resolved Hide resolved
@wslulciuc wslulciuc enabled auto-merge (squash) June 24, 2022 17:41
@wslulciuc wslulciuc disabled auto-merge June 24, 2022 18:15
@wslulciuc wslulciuc merged commit 2282f97 into main Jun 24, 2022
@wslulciuc wslulciuc deleted the add-copyright branch June 24, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants