Closed JeffreyGreenII closed 3 years ago
Sorry I can't review PRs that combine functional changes with linting/cleanup. It is too difficult/time consuming to understand what is going on. If you can start with just one or the other, this will be much easier.
Sorry I can't review PRs that combine functional changes with linting/cleanup. It is too difficult/time consuming to understand what is going on. If you can start with just one or the other, this will be much easier.
Sure thing
@JoshData Removed the general lint fixes, per your request. The only lint changes remaining are import sorts. The biggest problem is that the Travis CI won't pass because it's using python test/run
instead of pytest test
Fixed the Travis/CI build by using pytest in the test/run script
Updated the travis yaml to run pytest instead of using the old file.
I see that you've added a new script to start off the scrapers, but what I see is:
run
script is still there - it's not replaced, so with this PR we'd have two ways to do the same thing (i.e. twice the maintenance).So unless we have 100% replacement of the run script, there’s no benefit to update this? Because I’m willing to add coverage for all of these and completely replace the run function (which you can find in another PR I’ve already worked on in my fork), but figured that you, the maintainer, wouldn’t allow a complete overhaul. I figured an incremental approach would preferred by the maintainers.
Also, what lint changes? Like I mentioned the only thing I’ve changed is the sort of the imports.
So can I get a list of instructional changes you’d need implemented?
I’m guessing a bit based on your complaints about the PR, but if I implement the following changes, you’d be willing to review this PR?
If I’m guessing correctly, that seems like a large ask to begin implementing a more maintainable CLI, but I could be wrong. If that is what’s required, I’ll go ahead and close this PR.
Those weren't complaints. You proposed a change without giving any context, summary, or plan, so I was just stating what I saw in terms of what was done and what wasn't done.
Tbh, I don't really care about the tests/coverage, so that's not necessary. (I don't know if anyone else will have an opinion about that, but probably not.)
I am also OK with incremental.
But a command to invoke the project is too important to go undocumented (in both the README and on relevant wiki pages), and since we don't need two ways to invoke a scraper, for any scraper where it is implemented it should replace the old run script method. That seems to me like a reasonable minimum for merging a change like this. Undocumented and duplicative code is not going to benefit anyone else and will likely create a headache for me down the road.
Summary
./run
commandUnit tests are all passing using Pytest: