Closed mzjp2 closed 4 years ago
Better use of version using click.version_option rather than building a custom version ourselves.
How have I never seen this before! You have saved me from proliferating this mistake through more projects!
production python shouldn't have print's
🤣 Completely correct, I get a bit heavy with my prints sometimes. I still have one in the _vprint method of core.py#74
Had to fix a test for the version, but I'd suggest not testing that aspect anyway (maybe delete that test?) because it's more testing click than testing your own stuff.
I was on the fence and almost didn't add it. I think I was in a bit of a testy mood at the time. I completely agree that It's more or less testing the framework rather than my implementation.
Move import os and import sys
Nice catch, I think this was just me quickly adding those without the intention of keeping them. I originally kept them in the same place to delete them all together. Then I opted to wrap them up in the verbose flag, and never cleaned up, thanks for the cleanup.
Is there a reason you build a custom CLI instead of attaching to kedro's? So kedro find <...>?
I was on the fence again here, You can see my struggle to make a decision remains in setup.py#30. I opted against putting it in the kedro cli as I don't think the cli adds a ton of benefit for this particular project. I was trying to avoid cluttering up the kedro cli for any users. The cli is mostly there as a helper, and for me give users an easy command to run before submitting questions.
✨ You're Awesome Zain, Thanks for the contributions!
How have I never seen this before! You have saved me from proliferating this mistake through more projects!
🚀 It's an awesome option, click
is so great! 😀
I was on the fence again here, You can see my struggle to make a decision remains in setup.py#30. I opted against putting it in the kedro cli as I don't think the cli adds a ton of benefit for this particular project. I was trying to avoid cluttering up the kedro cli for any users. The cli is mostly there as a helper, and for me give users an easy command to run before submitting questions.
That makes sense, thanks for clearing up the justification behind that. 😄
✨ You're Awesome Zain, Thanks for the contributions!
Thank you, and you're awesome for creating this plugin. 💯
This PR tackles the following:
version
usingclick.version_option
rather than building a custom version ourselves.print
toclick.echo
(production python shouldn't haveprint
's)import os
andimport sys
to top of the file. There's no performance improvements to importing them within thecli
function becauseimport click
importsos
andsys
anyway, so there's no performance gain (Python has smart import loading, so won't load it twice).Tests:
version
, but I'd suggest not testing that aspect anyway (maybe delete that test?) because it's more testing click than testing your own stuff.Question
Is there a reason you build a custom CLI instead of attaching to
kedro
's? Sokedro find <...>
?