search

LLNL / scraper

Python library for getting metadata from source code hosting tools
MIT License
49 stars 23 forks source link

GitHub Cleanup #46

Closed LRWeber closed 4 years ago

LRWeber commented 4 years ago
leebrian commented 4 years ago

Neat, thanks for this cleanup, I especially think the readme changes will help new users.

It looks like some of your files are changing single quotes to doubles. When diffing it takes a little more time to filter out the lines with real changes vs the ones that change quotes.

If you think it’s valuable to use doubles, could you maybe send a pr to change the entire project. I don’t know Ian’s preference as that’s probably most important.

I think using the same quotes throughout the project will help with maintainability and to attract more great contributors.

LRWeber commented 4 years ago

I don't really have a personal preference one way or the other for singles vs doubles. That file, as it was, had a mixing of both. I changed to doubles because it looked like they were the majority. However, I see now that other Python files in the repo tend to favor single quotes!

I can certainly make the change the other direction. I agree that there should be consistency across the project. Though I'll similarly defer to @IanLee1521 for recommendations as to what that style should look like.

IanLee1521 commented 4 years ago

I think part of this is that we at LLNL started playing with using Black as our linter... I think that is what we should do, but I agree that it would be good to split that bit out to a separate PR and do it on everything.

@LRWeber , can you do that?

IanLee1521 commented 4 years ago

And if we merge that first, it should clear up some of the issues here in this pull request that confuse the differences.

LRWeber commented 4 years ago

Agreed, that will simplify the cleanup (and future maintenance). I'll work on that now!

IanLee1521 commented 4 years ago

🎉

leebrian commented 4 years ago

That will be cool. I think CI runs flake8 for peps and whatnot. Can it also run Black to check for future checkins?

LRWeber commented 4 years ago

It may be worth noting that if both are running, we may have to add some exclusions to our flake8 options. For the most part, code that clears black should be cleared by flake8's default formatting rules as well, but I've seen a few exceptions.

LRWeber commented 4 years ago

If it's an automatic check, we may also want to run it as black --check .. Otherwise, it will modify the files by default. It may be better to return success/failure like flake8 currently does rather than making changes out of our control.

IanLee1521 commented 4 years ago

@LRWeber -- Since this is all your code, do you mind taking a look at the conflict and resolving when able, now that I merged #47 ?

LRWeber commented 4 years ago

@IanLee1521 -- Conflicts should all be resolved, and I added a few more improvements.