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

Repo improvements #2

Merged
merged 13 commits into from
Oct 3, 2022

Conversation

markreidvfx
Copy link
Member

@markreidvfx markreidvfx commented Aug 6, 2022

This pull request address the following the tasks in #1

  • Add Code of Conduct and Contributing docs (matching unreal plugins style)
  • Fix Badge path in Readme
  • Add issue and pull request templates
  • Fix CI pip install OTIO URL
  • Add CodeCov CI support
  • Add Dependabot
  • Small Readme improvements

CodeCov/Dependabot might need to be enabled on the repo settings if it's not already.

Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@46f749e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #2   +/-   ##
=======================================
  Coverage        ?   90.03%           
=======================================
  Files           ?        3           
  Lines           ?     1114           
  Branches        ?        0           
=======================================
  Hits            ?     1003           
  Misses          ?      111           
  Partials        ?        0           
Flag Coverage Δ
unittests 90.03% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Bump readthedocs-sphinx-ext from 2.1.5 to 2.1.8
Bump myst-parser from 0.17.2 to 0.18.0

Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

I made some small comments. but it looks pretty good to me.


Each PR should link at least one issue, in the form:

```Fixes #123```

Choose a reason for hiding this comment

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

Suggested change
```Fixes #123```
Fixes #123

I guess that was copied from OTIO repo. Using triple backquotes here will lead to Fixes #123 which GitHub won't be able to understand and so won't link the PR to the issue. Eventually it also needs to be fixed in OTIO.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this was just copied from the main OTIO repo.

README.md Outdated
------------

* [OpenTimelineIO](https://github.com/AcademySoftwareFoundation/OpenTimelineIO) > 14.1 (latest git version currently recommended)
* [pyaaf2](https://github.com/markreidvfx/pyaaf2) >= 1.4.0

Choose a reason for hiding this comment

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

Is it necessary to list the requirements? They are already set in the setup.py. I find that redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally don't like having to dig into a project files to find such basic information, but I can remove it if preferred.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of making it clear that you need these two packages (esp pyaaf2), but let's omit the version info here, since that may change often.

Copy link
Member Author

@markreidvfx markreidvfx Sep 3, 2022

Choose a reason for hiding this comment

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

good idea, I'll remove the version numbers.

README.md Outdated
Comment on lines 8 to 9
Overview
--------

Choose a reason for hiding this comment

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

Suggested change
Overview
--------
# Overview

I would recommend using markdown headers. It will render more nicely.

-------------

If you have any suggested changes to the otio-aaf-adapter,
please provide them via [pull request](../../pulls) or [create an issue](../../issues) as appropriate.

Choose a reason for hiding this comment

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

This really works? If so I just learned something!

Copy link
Member Author

@markreidvfx markreidvfx Aug 28, 2022

Choose a reason for hiding this comment

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

yes it does, saw some other repos doing it. Was very surprised too that it works!

setup.cfg Outdated
@@ -23,7 +23,7 @@ ignore =
W504

[tool:pytest]
addopts = --cov=./ -W ignore::DeprecationWarning
addopts = --cov=otio_aaf_adapter -W ignore::DeprecationWarning

Choose a reason for hiding this comment

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

Suggested change
addopts = --cov=otio_aaf_adapter -W ignore::DeprecationWarning
addopts = --cov=otio_aaf_adapter

It is generally better if CI logs (or even fails, -W error::DeprecationWarning) when DeprecationWarnings are encountered.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is like that in the template repo, but I agree

Copy link
Member

Choose a reason for hiding this comment

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

We could try turning that off to see if it passes. I suspect that since we're running against such a wide range of Python versions that some deprecations are unavoidable...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already turned it off ignore and all the tests still pass. I didn't try -W error::DeprecationWarning though, I thought that would be overkill for now.

Comment on lines +19 to +22
<!--
Important: If this is your first contribution to OpenTimelineIO, you will need to submit a Contributor License Agreement. For a step-by-step instructions on the pull request process, see
https://github.com/AcademySoftwareFoundation/OpenTimelineIO/tree/main/CONTRIBUTING.md
-->

Choose a reason for hiding this comment

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

Should this stay here? Will CLA/CCLA be required for the adapters repos?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is my assumption that it is needed for officially supported adapters.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed the pros/cons among the TSC and decided that yes, we would like to have the same CLA (and DCO) requirements for repos in the OpenTimelineIO GitHub organization. That will ensure that all the approvals and such are uniform between these and the core repo in the ASWF org. If a particular repo needs/wants to follow different rules, it can just live in a different org.
Does that sound reasonable to you two @markreidvfx and @JeanChristopheMorinPerso ?

Copy link
Member Author

@markreidvfx markreidvfx Sep 3, 2022

Choose a reason for hiding this comment

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

Sounds reasonable to me!
I don't think DCO has been activated for this repo yet, I believe its somewhere in the repo settings.
Having EasyCLA available for plugin repos at some point would be great too.

Copy link
Member

Choose a reason for hiding this comment

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

I have no problem with DCO and CLAs in general, but I do find that they are kind of a barrier of entry in some cases. I find that it's a lot of bureaucracy to contribute to an adapter. But I guess there is some legal questions regarding adapters that are moved out of the OTIO repo and stuff like that? Anyway, if the TSC decided to go with CLAs+DCO, then that's the way it is. I'll still happily sign the CLAs and contribute :)

Comment on lines 30 to 34
5. Compiler information:
on Mac, type `clang -v`, and paste the results here
on Linux, type `gcc -v`, and paste the results here
on Windows, type `cl`, and paste the results here.
If you are unable to determine your compiler via these commands, please indicate that here.

Choose a reason for hiding this comment

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

I don't think it's relevant here since pyaaf2 is pure Python? Or potentially we keep it here just in case someone simply installs otio-aaf-adapter without actually installing OTIO (which would mean OTIO would be installed by otio-aaf-adapter)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can get rid of this, I just missed it when copying the file over from the main OTIO repo

Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
* Mark Reid ([markreidvfx](https://github.com/markreidvfx))
* Stefan Schulze ([stefanschulze](https://github.com/stefanschulze))
* Stephan Steinbach ([ssteinbach](https://github.com/ssteinbach))
* Tim Lehr ([timlehr](https://github.com/timlehr))
Copy link
Member

@jminor jminor Sep 2, 2022

Choose a reason for hiding this comment

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

Could you add these people also?

* Shahbaz Khan ([shahbazk8194](https://github.com/shahbazk8194))
* Freeson Wang ([freesonluxo](https://github.com/freesonluxo))
* Andrew Moore ([andrewmoore-nz](https://github.com/andrewmoore-nz))

Copy link
Member Author

Choose a reason for hiding this comment

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

will do!

Copy link
Member

@jminor jminor left a comment

Choose a reason for hiding this comment

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

This is looking fantastic! There's just a couple of tiny changes left as comments.

Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Mark Reid <mindmark@gmail.com>
@markreidvfx
Copy link
Member Author

Added the requested changes to the pull. Everything should be addressed unless I missed something :p

@markreidvfx markreidvfx requested a review from jminor September 13, 2022 20:20
Copy link
Member

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

LGTM

@ssteinbach ssteinbach merged commit 8bde894 into OpenTimelineIO:main Oct 3, 2022
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.

5 participants