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

mv authors #410

Merged
merged 3 commits into from Apr 22, 2017
Merged

mv authors #410

merged 3 commits into from Apr 22, 2017

Conversation

meeuw
Copy link
Contributor

@meeuw meeuw commented Apr 20, 2017

Description

This patch is used by Fedora to get AUTHORS and SPONSORS in /usr/lib/pythonN/site-packages/mycli instead of /usr/lib/pythonN/site-packages.

Checklist

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).

@tsroten
Copy link
Member

tsroten commented Apr 20, 2017

Thanks for figuring this out. However, I think the best practice is to not include AUTHORS and SPONSORS in package_data at all.

They should be in the manifest file (and kept in the project root directory, not in the Python package itself).

@tsroten
Copy link
Member

tsroten commented Apr 20, 2017

Oh wait, we need them for the credits when we load mycli. Hmm. Mycli really shouldn't load things outside of its package. But, I don't like the AUTHORS file not being in the project root directory.

Let's think through a better way to do this.

@tsroten
Copy link
Member

tsroten commented Apr 20, 2017

Ok, what about data_files where we can control where they are installed? https://docs.python.org/3.4/distutils/setupscript.html#installing-additional-files

Maybe use the relative path mycli as the installation directory? Then we'll need to update the thanks_picker to look in the package itself.

Edit: Apparently, data_files is not reliable and shouldn't be used anymore.

@codecov-io
Copy link

codecov-io commented Apr 21, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@818f5cd). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #410   +/-   ##
=========================================
  Coverage          ?   72.67%           
=========================================
  Files             ?       29           
  Lines             ?     2543           
  Branches          ?        0           
=========================================
  Hits              ?     1848           
  Misses            ?      695           
  Partials          ?        0
Impacted Files Coverage Δ
mycli/main.py 61.36% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 818f5cd...393ee31. Read the comment docs.

@meeuw
Copy link
Contributor Author

meeuw commented Apr 21, 2017

I admit this is a bit lame, but is it acceptable to have AUTHORS.rst and SPONSORS.rst to redirect to the text files in our mycli directory?

@amjith
Copy link
Member

amjith commented Apr 21, 2017

Instead of adding a link inside the AUTHORS to mycli/AUTHORS why not just create a symlink?
We move AUTHORS and SPONSORS inside mycli folder and then symlink it to the outside folder?

@meeuw
Copy link
Contributor Author

meeuw commented Apr 21, 2017

I don't know if it is important but last time I've used Windows/NTFS it didn't support symlinks.

And I don't know how github handles symlinks, I'm pushing a test commit...

@meeuw
Copy link
Contributor Author

meeuw commented Apr 21, 2017

Yikes, github doesn't follow symlinks:
https://github.com/meeuw/dmtest

@meeuw meeuw force-pushed the feature/use_authors branch 2 times, most recently from 771d107 to 2ce2a0b Compare April 21, 2017 18:53
@tsroten
Copy link
Member

tsroten commented Apr 22, 2017

@meeuw I removed the project_root variable. This pull request looks good to me. Anything holding it back now? Or are we good to merge?

@meeuw
Copy link
Contributor Author

meeuw commented Apr 22, 2017

I've thought hard about alternatives but I cannot think of anything better than this, so -- good to merge!

@tsroten tsroten merged commit 1b44d31 into master Apr 22, 2017
@tsroten tsroten deleted the feature/use_authors branch April 22, 2017 18:58
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

4 participants