Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Simplify notebooks' setup by using a configuration file #12

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kouloumos
Copy link

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.

allow for a single point of configuration by:
- moving all  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.
changing from absolute to relative references makes references more
future-proof and allows for cross-notebook navigation when reading
outside of github (e.g locally).
side effect: fix broken links from previous notebooks' renames
@DariusParvin
Copy link
Collaborator

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?

@kouloumos
Copy link
Author

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.

@DariusParvin
Copy link
Collaborator

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.

@liorwavebl
Copy link

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

@DariusParvin
Copy link
Collaborator

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.

@kouloumos
Copy link
Author

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!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants