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

Update documentation #62

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Update documentation #62

merged 4 commits into from
Jan 9, 2024

Conversation

twsearle
Copy link
Collaborator

Description

Update the documentation prior to public release of nemo-feedback. Also add a copy of the google cpp linter and a corresponding ctest.

Merge of this PR will be considered as satisfying the requirement to sanitize nemo-feedback for ticket #60.

Issue(s) addressed

Part of #60

Impact

None expected.

Copy link
Collaborator

@s-good s-good 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 making these changes. I've scanned through the code and the text in the pull requests and issues. Everything mostly looks fine. Just a few comments:

  1. What is the status of cpplint.py? Are we OK to include this Google code?
  2. In the same directory of cpplint.py there is valgrind.supp. Is this code/text ours and should it have a copyright statement?
  3. Should the copyright dates be updated to 2024 (NB I noticed one instance of the date being 2022 in tests/add-linting-check/ci/CMakeLists.txt).
  4. Only the code files have the Met Office copyright at the top. Should the other files also have it?

@twsearle
Copy link
Collaborator Author

twsearle commented Jan 5, 2024

Thanks for making these changes. I've scanned through the code and the text in the pull requests and issues. Everything mostly looks fine. Just a few comments:

1. What is the status of cpplint.py? Are we OK to include this Google code?

2. In the same directory of cpplint.py there is valgrind.supp. Is this code/text ours and should it have a copyright statement?

3. Should the copyright dates be updated to 2024 (NB I noticed one instance of the date being 2022 in tests/add-linting-check/ci/CMakeLists.txt).

4. Only the code files have the Met Office copyright at the top. Should the other files also have it?

For cpplint.py please see the discussion on the sister PR in orca-jedi: MetOffice/orca-jedi/pull/63.

My understanding is that copyright notices are not really a legal requirement - in the UK we get copyright automatically - so we have no worries on that score. As to internal Met Office policy regarding copyright notices, I am not 100% sure. I believe a copyright notice in the README clearly visible with the work is sufficient to inform users. General advice seems to be as long as the work is not expected to be "split up" and still useful a notice there should be sufficient.

In terms of the copyright year, again, my understanding is that this is more of a notice to users than a legal thing. It is usually either the year that the file was first produced, the year the file was last modified, or a range including both these dates. I have not received any consistent advice on what this ought to be, however again I believe this to be a question of internal style. Probably @matthewrmshin is the person I know with the best knowledge of general practises with copyright and open source licensing. He may have something to add. Given this seems like extra admin for no real value (the commit history on the repo contains the more accurate record of when the code was changed, who changed it, and when features were added), I opted to just pick a year and put it at the top of every file.

For extra safety and to follow the precedent set by other repositories in the JEDI project, I opted to add copyright notices to the top of every source code file. JCSDA does not put copyright notices on configuration files, such as this yaml file for example. The valgrind.supp file is a configuration file for valgrind - it doesn't really contain anything bespoke to our project - it is just used to suppress expected errors when using valgrind to try and detect memory errors.

@twsearle
Copy link
Collaborator Author

twsearle commented Jan 5, 2024

@s-good one more thing that might make this a bit easier is comparing what we have to other existing Met Office public github repositories: https://github.com/MetOffice?q=&type=public&language=&sort=

@matthewrmshin
Copy link
Collaborator

cpplint.py is safe to include as long as its licence is intact.

I believe @twsearle is correct on copyright statements.

In the past, (when I was managing FCM, Rose and Cylc, for example), I would introduce a happy new year PR to a repo before the first proper PR is merged in a new year. In the happy new year PR, I would use a script to update the date to the current year in every relevant copyright statements. This was before the days of GitHub Actions.

It looks like:

README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Tested in my environment. Works as expected.

Copy link
Collaborator

@s-good s-good 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 making the updates.

@twsearle twsearle merged commit 0162b28 into develop Jan 9, 2024
2 checks passed
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