Closed mzjp2 closed 3 years ago
@mzjp2 Thanks Zain, I was working on exactly the same thing haha. I'm going to start reviewing now/tomorrow.
@mzjp2 Thanks Zain, I was working on exactly the same thing haha. I'm going to start reviewing now/tomorrow.
Super sorry - I hadn't seen your comment on the issue. Feel free to push anything to my branch, I've allowed push access to maintainers. And please add yourself as a co-author on this PR if it does get merged in :)
@mzjp2 oh no problem at all, I'll commit in to here if necessary. Happy to have the contributions you have made so thank you!
I think this is really cool (sorry for geeking out), but having a red cross on your PR + emails (if you signed up to GH notifications) telling you your PR build failed because of a linting or testing issue.
If you're the GH repo admin/mantainer of this repo and want to take it a step further, you can also enforce that the builds pass before allowing a merge (admins can bypass this if urgent), by doing the following:
master
if it already exists)I'd say leave this off for now, but something that you might want to enforce further down the line.
@mzjp2 Yeah I think eventually we will want to do that on PRs for sure. Setting up solid package management, like in this PR will, will make that easier for those making the PRs
This is fantastic, thank you @mzjp2. I'll let you and @ryan-wolbeck work out the details, but please do write up a contributor guide so that I can read it and stop fucking things up :)
This is fantastic, thank you @mzjp2. I'll let you and @ryan-wolbeck work out the details, but please do write up a contributor guide so that I can read it and stop fucking things up :)
😂 haha - hopefully this is not unfuckable with make install
- make lint
and make test
being all you need to do, with all the benefits of an isolated virtual environment happening in the background without being exposed to it!
just reading thru the PR now, really increadible work @mzjp2
Apologies in advance for the massive PR - I touch almost every feel. Let me try to summarise:
poetry
(including all the stuff we had to do manually usingsetup.py sdist
andtwine
). That's all taken care of bypoetry build
andpoetry publish
.make lint
) that does a whole host of things:isort
(import sorting) all the files, which made a couple of diffsand
flake8` on the files, which lead to thousands of errors that I fixed manually where I could (and where it was simple a cosmetic change), and ignored errors where I didn't know what to do. I think we can slowly fix any remaining ignored errors as and when we come across them.ngboost/tests
totests/
and movedconftest.py
totests/conftest.py
following best practice Python module layouts. This means that tests aren't bundled with the package when the wheel file is pushed to PyPI - which is a good thing.This is what
make lint
looks like with errors:The end goal here is twofold:
make lint
andmake test
after you're done and hopefully have everything fixed (isort/black), fixup any flake or pylint errors, then push your changes to the repo and open a PRmake lint
andmake test
on the PR, and blocks merges unless they pass.The best way to review this PR, is likely to
git checkout
it and play around within. Things are hopefully much simpler and I'm happy to:CONTRIBUTING.md
guide in a follow-up PR