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

RESTX Rename #2

Merged
merged 7 commits into from
Jan 12, 2020
Merged

RESTX Rename #2

merged 7 commits into from
Jan 12, 2020

Conversation

SteadBytes
Copy link
Contributor

@SteadBytes SteadBytes commented Jan 9, 2020

Addresses #1

TODO:

  • Rename references to Flask-RESTPlus, flask-restplus e.t.c to Flask-RESTX, flask-restx e.t.c
  • Remove Flask-RESTPlus logo
  • Add new Flask-RESTX logo
  • Remove AUTHORS.rst
  • Remove CHANGELOG.rst
  • Update LICENCE
    • Remove Akamai
    • Add another Modifed work Copyright line (see questions)
  • Update setup.py author and author_email (see questions)

Questions:

  • Which name should go into setup.py as the author and into the licence copyright? Are the other maintainers happy for my name to be used here (as I'm making these changes)?

@SteadBytes SteadBytes force-pushed the restx-rename branch 3 times, most recently from defb5c1 to 5952151 Compare January 9, 2020 19:51
@apryor6
Copy link

apryor6 commented Jan 10, 2020

Woo hoo for moving forward!

@andreykurilin
Copy link
Contributor

Which name should go into setup.py as the author and into the licence copyright? Are the other maintainers happy for my name to be used here (as I'm making these changes)?

python-restx team?

@ziirish
Copy link
Contributor

ziirish commented Jan 10, 2020

Agreed, "python-restx team" is good as author as from now on, this is a community driven project.

@SteadBytes
Copy link
Contributor Author

SteadBytes commented Jan 10, 2020

Sounds good to me! I just want to check if "python-restx team" is valid as the copyright line in the licence i.e. Modified work Copyright (c) 2020 python-restx team i.e. do we need to be some sort of legal entity? Is anyone (more knowledgeable of licencing minutia than myself) able to comment on this?

Edit: I've found an example from the chromium project https://github.com/chromium/dom-distiller/blob/d16a68c1b885f679ae1266a9eeee66e69c12e67c/javatests/org/chromium/distiller/OpenGraphProtocolParserAccessorTest.java#L1

Should we follow suit with python-restx Authors ?

@SteadBytes
Copy link
Contributor Author

We also need an author_email for setup.py - any suggestions?

@andreykurilin
Copy link
Contributor

@SteadBytes actually, only 3 fields are required: name, version, and packages. We can just drop author_email.

Also, based on first clause of BSD license I'm not sure that we can remove Akamai copyright: Redistributions of source code must retain the above copyright notice. I guess the only available "workaround" is to revert an original commit that adds copyright

@SteadBytes
Copy link
Contributor Author

@andreykurilin author_email is required if author is specified, so we either have both or neither.

I defer to @j5awry on the Akamai licence bit, but he stated that he never made any copyrightable commits to the code whilst working at Akamai and won't be moving forward either.

@andreykurilin
Copy link
Contributor

@SteadBytes

author_email is required if author is specified, so we either have both or neither.

oops, I did not know this. But after googling, it looks like not true anymore - python/cpython#17388 (setuptools->distutils->cpython). I guess we can specify invalid email for now (if there are no other options) and remove it as soon as new python packages will be available.

I defer to @j5awry on the Akamai licence bit, but he stated that he never made any copyrightable commits to the code whilst working at Akamai and won't be moving forward either.

yeah, but technically current change looks like 'forking with loss of one of the original authors'. At least, it is better to do it separately.

@SteadBytes
Copy link
Contributor Author

@andreykurilin
Ah yeah, that's change is quite recent - on my Python 3.7 install setup.py check throws a warning:

python3 setup.py check
/usr/lib/python3.7/distutils/dist.py:274: UserWarning: Unknown distribution option: 'dev_require'
  warnings.warn(msg)
/usr/lib/python3/dist-packages/setuptools/dist.py:474: UserWarning: Normalizing '0.13.1.dev' to '0.13.1.dev0'
  normalized_version,
running check
warning: check: missing meta-data: if 'author' supplied, 'author_email' must be supplied too

As long as it doesn't affect building the project (i.e. it's only affect is showing that warning) then I'm happy to remove it - good spot thankyou!

I see your point, I will wait for @j5awry to confirm how to proceed with this it was originally introduced by/for him. The commit message has a clear explanation of why the change is taking place so as long as the explanation is actually valid then I think we're ok. That said, I'll make sure to have confirmation from @j5awry before proceeding further 👍

README.rst Outdated Show resolved Hide resolved
@andreykurilin
Copy link
Contributor

LGTM :) Thanks for pushing this forward!

@j5awry
Copy link
Contributor

j5awry commented Jan 10, 2020

Legally, it's fine. There were 0 commits done with Akamai attached to it. Akamai normally only cared about commit level (if i made a commit, add in "akamai copyright"). They normally don't do a top level.

In fact, doing a giant top level with a bunch of entries is highly irregular.

@j5awry
Copy link
Contributor

j5awry commented Jan 10, 2020

Essentially, Akamai can't claim copyright when no copyrightable work by an Akamai employee is done. If you'd all rather, i can push the specific commit and have it in the commit message.

@andreykurilin
Copy link
Contributor

I do not pretend to be a legal expert and I guess Akamai is not offended here, but it would be nice to save all this context into the history of the project. That is why I raised this question.

Please accept my apologies, @SteadBytes, I used to review PRs on GitHub where people forgot to add proper commit messages to all commits. Thanks to add a good commit message for a change to LICENCE!

@ziirish
Copy link
Contributor

ziirish commented Jan 10, 2020

The tests are failing due to the lack of author_email:

warning: Check: missing meta-data: if 'author' supplied, 'author_email' must be supplied too

I tried locally with a dummy author_email and the tests succeed.

@andreykurilin
Copy link
Contributor

@ziirish I guess we can modify https://github.com/python-restx/flask-restx/blob/master/tasks.py#L160 with warn=False to avoid CI failures. Another thing, as you mentioned, is setting author_email, but there is no team specific email yet...

@SteadBytes
Copy link
Contributor Author

Thanks for clarification @j5awry - I'll leave the licence changes in 👍 No need to apologise @andreykurilin, one cannot be too careful when it comes to licencing!

Since the warning for author_email has been officially removed as @andreykurilin helpfully pointed out, I'm happy to adjust https://github.com/python-restx/flask-restx/blob/master/tasks.py#L160 to either: remove it entirely or make it output the warning but not fail the build. It's also somewhat confusingly named readme_check when it's testing setup.py 🤔

Of course the other option is to include the email, I'm not in favour of using a placeholder email. I see several options:

  1. Use one of our own email addresses
  2. Create a team email account (i.e. a Gmail account python-restx@gmail.com) and share access amongst the core team
  3. Similar to point 2 but done 'properly' using a domain (i.e. G Suite)
    • Major downside for this is cost, hence I'm not in favour

@ziirish
Copy link
Contributor

ziirish commented Jan 11, 2020 via email

@SteadBytes SteadBytes mentioned this pull request Jan 11, 2020
@SteadBytes
Copy link
Contributor Author

Ok awesome thanks @ziirish , looks like no author_email will be fine 👍

Copy link
Contributor

@ziirish ziirish left a comment

Choose a reason for hiding this comment

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

LGTM

@SteadBytes
Copy link
Contributor Author

Brilliant, thanks for adding the logo @ziirish 😄 I'll give it one more check over, fixup the commits and then merge 👍

Run regex replace to cover the following:

Flask-RESTPlus -> Flask-RESTX
Flask-RESTplus -> Flask-RESTX (there were a few typos)
flask-restplus -> flask-restx
restplus -> restx
At the time of forking, this file is very out of date. It also
provides little benefit over using git to retrieve a list of
commit authors.
Removing in favour of solely using GitHub release notes.
Previously, clauses were added to the licence to allow John Chittum
(@j5awry) to contribute to the project whilst working at Akamai.
During this time, he made no copyrightable code commit and as of
Jan 17th 2020 he will no longer be working for Akamai. As such,
there is no need for Flask-RESTX to continue with the licence
additions.
Change project authorship to "python-restx Authors"
and update LICENCE/setup.py accordingly to reflect
ownership of the new fork. The author_email field
has been removed from setup.py as it is no longer
a required field python/cpython#17388
and setting up a python-restx team email is not a
task we're looking to take on at the moment.
setup.py test has been deprecated pypa/setuptools#1684
in favour of twine test. This commit removes the existing use of
setup.py test when checking the README has been parsed properly
and replace it with twine test.
@SteadBytes SteadBytes marked this pull request as ready for review January 12, 2020 17:57
@SteadBytes SteadBytes merged commit 3da9017 into master Jan 12, 2020
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