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

Fill out setup.py #68

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fill out setup.py #68

wants to merge 3 commits into from

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Feb 10, 2020

#66

@altendky
Copy link
Contributor Author

If you have a more specific minimum version than 3, let me know and I can fill it out.

setup.py Outdated
install_requires=['six', 'dateutils'],
url="http://vsjha18.github.com/nsetools",
packages=find_packages(),
long_description=readme,
long_description=(pathlib.Path(__file__).parent / 'README.md').read_text('utf-8'),
Copy link
Owner

Choose a reason for hiding this comment

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

Have you verified whether this particular change works?

Copy link
Owner

Choose a reason for hiding this comment

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

I am asking this because while building I have to setup.py one directory above to build it properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not verified in this case. Do you mean that with this change you have to build one directory up? Or that you do that for other reasons? This does sound a bit odd either way.

__file__: is the path to the file where it is written, setup.py in this case
pathlib.Path(__file__): is a pathlib.Path object referring to this file
pathlib.Path(__file__).parent: refers to the directory containing this file
pathlib.Path(__file__).parent / 'README.md': refers to the README.md next to this file
(pathlib.Path(__file__).parent / 'README.md').read_text('utf-8'): reads the readme into a string decoding from utf-8

Copy link
Owner

@vsjha18 vsjha18 Feb 19, 2020

Choose a reason for hiding this comment

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

when I run the packaging command, I move setup.py up level above. Yes it is awkward but it was done long back. Before your change, "long_description" was inside setup.py itself. But in your change you are using the contents of README.md to populate long_description.

I will typically clone the project in nsetools directory. And move the setup.py one level above.

setup.py
nsetool/
     bases.py
     nse.py
     README.md

I know it's not a good design but I don't want to mess with the build workflow.

Can you change this code:
(pathlib.Path(__file__).parent / 'README.md').read_text('utf-8')
in such a way that it looks for README.md at
$PWD/nsetools/README.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alrighty, that's interesting. :] Definitely a good thing to change at some point, but sure, not in this PR. How about I make it look in .parent and .parent / 'nsetool'? Whichever it finds it will use. When you stop moving setup.py then the check-both code can be removed. I don't like depending on cwd because it really shouldn't matter and it doesn't seem like it needs to.

I could probably find some time to PR a good project layout. Can the tests be run in an automated manner? Could also setup GitHub Actions testing.

Copy link
Owner

@vsjha18 vsjha18 Feb 19, 2020

Choose a reason for hiding this comment

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

I run tests using this command.
nosetests
You need to install "nose". Just run the command from root of the project, it will detect the tests.

If are interested in coverage then
nosetests --with-coverage --cover-package nsetools.nse --cover-branch
Make sure you have "coverage" installed.

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.

2 participants