bitcoin-tx-tutorial icon indicating copy to clipboard operation
bitcoin-tx-tutorial copied to clipboard

Simplify notebooks' setup by using a configuration file

Open kouloumos opened this issue 2 years ago • 7 comments

I believe that way that the taproot-workshop is handling the reference to the bitcoin core directory is much more elegant and simpler for the user than having to redefine it in every notebook. But this can only be done when the notebooks are in the root directory of the project.

In case there is no strong preference to the encapsulation of each chapter in its own directory, this PR allows for a single point of configuration by:

  • moving all the notebooks to the root dir while switching to a <chapter_number>.<sub-chapter_number> prefix to keep the structure.
  • adding a configuration file config.ini in which the user can define their Bitcoin Core directory
  • adding setup.py as a single import that takes care of all the configurations required for the initial setup of the notebook.

kouloumos avatar Apr 02 '23 15:04 kouloumos

Thanks @kouloumos! This way is definitely more elegant and easier to maintain going forward 👍

I noticed that the code execution outputs seem to have been removed from the notebooks. Was this intentional? I prefer having them there because you can still see the example outputs as a useful reference without having to run the scripts. What do you think?

DariusParvin avatar Apr 04 '23 10:04 DariusParvin

I noticed that the code execution outputs seem to have been removed from the notebooks. Was this intentional?

I actually thought that those were committed by mistake 😅, that's why I tried to reset all the notebooks as part of my PR.

I prefer having them there because you can still see the example outputs as a useful reference without having to run the scripts. What do you think?

I don't have a lot of experience with jupyter notebooks, but that was the first time seeing this, that's why I got confused. I guess this is upon the author of the notebook, and because the notebooks here don't require the user to fill any blanks, it doesn't sound bad to have the outputs there. On the other hand, if this ever becomes interactive (I'm missing context on its indented usage), having the outputs might be confusing for the reader as you typically not expect the outputs to be there.

I'll push a change to restore the example outputs.

kouloumos avatar Apr 14 '23 13:04 kouloumos

Thanks @kouloumos ! Yeah the idea was the beginning parts are more instructional to show how the transaction works, then at the end it would be more interactive with an exercise. I thought it's still nice to have the beginning part as something executable though because then you can always experiment with it in your own way, as well as having it be a useful reference.

DariusParvin avatar Apr 17 '23 03:04 DariusParvin

I must say that this is very helpful PR! Please merge it to the main branch.

liorwavebl avatar Aug 26 '24 06:08 liorwavebl

hey @liorwavebl, thanks a lot for going through the tutorial and opening useful PRs! I appreciate your feedback on this PR too. At first I was hesitant to merge because I personally found it useful to review these notebooks with the outputs included for reference, but given your and @kouloumos 's feedback I am happy to merge it.

I notice though that there are some merge conflicts. If you're up for it, @liorwavebl , would you be able to have a go resolving them? then I can go ahead and merge.

DariusParvin avatar Sep 11 '24 05:09 DariusParvin

Thanks @liorwavebl for resurfacing this. @DariusParvin, re-adding the outputs has been on my to-do list, but it ended up slipping through the cracks 😅. I'll rebase the PR by the end of the week!

kouloumos avatar Sep 11 '24 16:09 kouloumos

hey @kouloumos, no rush but just bumping in case you forgot :)

DariusParvin avatar Oct 08 '24 03:10 DariusParvin