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.

Sign up for GitHub

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

Jump to bottom

Packaging #86

Merged
merged 46 commits into from
Sep 22, 2020
Merged

Packaging #86

merged 46 commits into from
Sep 22, 2020

Conversation

GjjvdBurg
Copy link
Contributor

@GjjvdBurg GjjvdBurg commented Jun 20, 2020 •
edited
Loading

This PR reshuffles the repo a bit to facilitate packaging the module as a Python package. I'd like to use this package in another project and having it on PyPI would make that a lot easier.

This PR makes the following main changes:

  • Install the node dependencies on installation of the package (provided npm is available). Node modules are installed inside the package directory, and the node call in simple_json.py is updated to handle this. This makes installation easier and works even when installed as a binary wheel.
  • Move the executable extract_article.py into the package, as a command line entry point. This is then installed as the readabilipy executable.
  • Add versioning
  • Removed requirements files as these are now incorporated in setup.py
  • Some small fixes to testing setup
  • Update travis config to new structure
  • Add a Makefile for project management

I'm happy to push to PyPI myself if you'd like.

GjjvdBurg added 15 commits June 20, 2020 16:43
@GjjvdBurg
Remove unnecessary __init__
7a30b99
@GjjvdBurg
Add setup.py with dependencies and include cli
0995be7
@GjjvdBurg
Move js code in a javascript dir inside the package
dc887d9
@GjjvdBurg
Remove __init__ in tests directory
08564bf 
Gives a path issue when using pytest
https://stackoverflow.com/a/41752043
@GjjvdBurg
Fix imports in test directory
9bb2542 
Tests is no longer a package, so relative imports need
to be replaced with absolute imports
@GjjvdBurg
Install node modules when installing package
0b0dd40 
This makes installation a lot easier as it takes
care of installing the required dependencies automatically,
provided npm is installed.
@GjjvdBurg
Adapt readability.js code to new package structure
92e50a9
@GjjvdBurg
Add makefile
13f356a
@GjjvdBurg
Update travis config to new package structure
4f21b8f
@GjjvdBurg
Remove package.json from project root
64056a0 
No longer needed, now in readabilipy/javascript
@GjjvdBurg
Fix package.json path in setup.py
2111c4b
@GjjvdBurg
Be more robust against npm not existing
d209949
@GjjvdBurg
Throw a warning if we don't have node
c0c9735 
This can happen if a user installs the package from a
binary wheel but doesn't have node installed.
@GjjvdBurg
Fix typo in travis config
2cc990a
@GjjvdBurg
Ensure dependencies are updated on Travis
ae9fc51 
Previous build failed because pytest was outdated
@GjjvdBurg GjjvdBurg marked this pull request as draft June 20, 2020 19:44
@GjjvdBurg GjjvdBurg mentioned this pull request Jun 20, 2020
Closed
@GjjvdBurg
Squashed commit of the following:
d38db7d 
commit 025128e
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:52:35 2020 +0100

    Ignore test directory with pylint

    Seems to be a known issue of pylint:
    https://stackoverflow.com/a/48025817

commit cf1e0eb
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:44:57 2020 +0100

    Ignore missing __init__ in pylint

    Seems to be no other way to tell pylint that test is not a package

commit ed367eb
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:38:38 2020 +0100

    Also check major version of Node is sufficient

commit ff4f1e7
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:29:48 2020 +0100

    Add code style checking to Makefile

commit 55a6c6d
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:29:38 2020 +0100

    Fix remaining code style errors

commit 4cb1c10
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:18:41 2020 +0100

    Fix erroneous missing init error

    A test directory is not a package, it shouldn't have an __init__.py.

commit 73c3c43
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:13:19 2020 +0100

    Fix some code style issues

commit abccad8
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:08:48 2020 +0100

    Remove debug statements from setup.py

commit 0b827aa
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:08:39 2020 +0100

    Use pre-installed nvm to update node

commit f27fc37
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 23:02:15 2020 +0100

    Make sure nvm installs the version we want

commit 14be23a
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:46:04 2020 +0100

    Try updating node version using nvm

commit ff20467
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:40:41 2020 +0100

    Check nvm remotes

    Seems like Travis has its own node installation, even in this
    python-language vm. We'll need to somehow bypass that.

commit af8c10b
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:31:03 2020 +0100

    check npm config

commit 5501e5e
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:13:25 2020 +0100

    This can't be it, right?

commit af6a402
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:08:04 2020 +0100

    Check node version

commit aefa005
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:04:30 2020 +0100

    List installed packages

commit dbe4c1c
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 22:02:06 2020 +0100

    Try removing old npm first?

commit be0c49f
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 21:57:01 2020 +0100

    Try updating npm?

    Node is stuck on version 8.12.0 somehow

commit d2bc162
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 21:51:16 2020 +0100

    Update node version

    jsdom requires a newer version of node

commit 9f8b599
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 21:30:20 2020 +0100

    Debugging travis

commit 757a318
Author: Gertjan van den Burg <[email protected]>
Date:   Sat Jun 20 21:18:50 2020 +0100

    Debugging travis
@GjjvdBurg GjjvdBurg marked this pull request as ready for review June 20, 2020 22:58
@jemrobinson
Copy link
Member

Hi @GjjvdBurg - it looks like this PR has been waiting for review for a while, but I've only just got a notification about it. It looks good, but it might be nice to talk it through in person (I've not thought about this code base for over a year, so I might need to get back up to speed). Shall we set up a Zoom call later this week?

@GjjvdBurg
Copy link
Contributor Author

Hi @jemrobinson! No worries, I actually only realised yesterday that I'd never requested a review on this PR, which is probably why you didn't get a notification. Happy to talk it through in person, I'll write you on slack to arrange a time.

@GjjvdBurg
Set version to None in setup.py
55e30ff 
This ensures that it is pulled from the __version__ file.
jemrobinson
jemrobinson requested changes Sep 15, 2020
View reviewed changes
Copy link
Member

@jemrobinson jemrobinson left a comment

Choose a reason for hiding this comment

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

Some thoughts/questions I had while looking this over. Overall I'm pretty happy with it though.

Makefile Outdated Show resolved Hide resolved
readabilipy/simple_json.py Outdated Show resolved Hide resolved
readabilipy/simple_json.py Outdated Show resolved Hide resolved
package-lock.json Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
Makefile Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
jemrobinson
jemrobinson reviewed Sep 21, 2020
View reviewed changes
make_release.py Outdated Show resolved Hide resolved
@jemrobinson jemrobinson merged commit 818bebb into alan-turing-institute:master Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jemrobinson jemrobinson jemrobinson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GjjvdBurg @jemrobinson