search

perrette / papers

Command-line tool to manage bibliography (pdfs + bibtex)
MIT License
146 stars 22 forks source link

main() defines things that are then used more than one place #42

Closed boyanpenkov closed 1 year ago

boyanpenkov commented 1 year ago

main() defined savebib(my) which was then used by a few functions in main -- this may be better off with global scope, since then import papers in another script would be able to use those functions.

There's a few of these.

boyanpenkov commented 1 year ago

Confirming all tests pass.

boyanpenkov commented 1 year ago

Confirming I see 101 passing and 2 skipped tests.

boyanpenkov commented 1 year ago

Confirming I see all tests pass after the last commit.

I now understand the code better, and will pause for a bit here until I hear from @perrette to see if this is a direction that makes sense to take. But the exercise here is 80% me understanding the code better, which is a win either way.

boyanpenkov commented 1 year ago

Actually, all set with pulling all these defs out of main; nifty.

Confirming tests pass.

perrette commented 1 year ago

Great that you've been working to improving the code. I think of course it makes sense. I'd argue the the top-most priority before moving things around remains to improve test coverage -- so that we don't break things unnoticed. Test coverage is pretty poor, as I was reminded during the last few days. About moving the addcmd etc outside of main: does it make it easier for you to read / understand them? The namespace separation is also a significant benefit. The reason why I had left them under main, and intertwined with the parser definition, was to have all related things in one place. The subparser feature gets pretty complex to handle in the code, and as I was writing it it was definitely better to have them inline. It may well be that it only gets confusing past this first phase -- as your modification suggests. I also had some trouble going through them now, nearly 6 years after I first wrote them.

boyanpenkov commented 1 year ago

Great that you've been working to improving the code. I think of course it makes sense. I'd argue the the top-most priority before moving things around remains to improve test coverage -- so that we don't break things unnoticed. Test coverage is pretty poor, as I was reminded during the last few days.

No problem; opened https://github.com/perrette/papers/issues/44

About moving the addcmd etc outside of main: does it make it easier for you to read / understand them?

A little bit -- I think the act of moving them and sorting out the issues was illuminating as much as anything; i.e. the implicit pickup of config that was not obvious.

The namespace separation is also a significant benefit. The reason why I had left them under main, and intertwined with the parser definition, was to have all related things in one place. The subparser feature gets pretty complex to handle in the code, and as I was writing it it was definitely better to have them inline. It may well be that it only gets confusing past this first phase -- as your modification suggests. I also had some trouble going through them now, nearly 6 years after I first wrote them.

Yeah, you're right -- the namespace here is a downside (and I realized that maybe some things I moved are not as general as I expected them to be...); but bib.py had things that were in the global namespace, and on first read, it was not clear what the differences were, conceptually.

Anyway, if this turns bad, we can move things back... again, 80% me understanding this better, and like 20% actual improvements.

perrette commented 1 year ago

Yeah the functions are really parts of main. However the bib.py file is more than 1300+ lines and I remember I long intended to make the main function in its separate file.py -- so that bib.py can be imported in some future application not directly related to the main cli. So I am taking this as an opportunity to do it (#43). Unless you spot some flaw or annoyance with it, I'll double down your move and accept that PR of mine. I'll take the chance to fix some bugs your PR introduced (check_install fails because parser, which lives under main() is not defined any more).

I agree that how config behaves is not intuitive and it will need to be improved.

boyanpenkov commented 1 year ago

Yeah the functions are really parts of main. However the bib.py file is more than 1300+ lines and I remember I long intended to make the main function in its separate file.py -- so that bib.py can be imported in some future application not directly related to the main cli. So I am taking this as an opportunity to do it (#43). Unless you spot some flaw or annoyance with it, I'll double down your move and accept that PR of mine.

Certainly no annoyance...

I'll take the chance to fix some bugs your PR introduced (check_install fails because parser, which lives under main() is not defined any more).

Oh, sorry about that; definitely didn't mean to inadvertently introduce a bug there!

I agree that how config behaves is not intuitive and it will need to be improved.

The way it is now, and the way #43 looks, the config itself is pretty tractable ("go down this list of options, read them."); the explicit thing that I discovered there was that the variable config was being silently passed around from the namespace it was in, instead of not explicitly as an argument and a return. So it definitely ran syntactically correctly; it was just hard to track what exactly had changed config and when.