fix: make bake cmd use default brownie-mix branch #917
Conversation
Previously, `brownie bake` would assume the master branch was in each brownie-mix repository. Now it checks the github api to verify the default branch first. Resolves #915
There was a problem hiding this comment.
Thanks a lot for implementing the requested changes and thanks so much for contributing!
Code-wise this lgtm, but I do have some remarks on style, see below.
Maybe @iamdefinitelyahuman can give some input as well on the style policy and if this needs to be changed. Approval from me 💯
brownie/project/main.py
Outdated
| project_path.parent.joinpath(project_name + "-mix-{}".format(default_branch)).rename( | ||
| project_path | ||
| ) |
There was a problem hiding this comment.
Style wise we generally use f-strings over .format() wherever possible and only use the latter when the string needs to be changed at a later point (as in line 56 and then 592). A simple string such as here should be written as f-string like f"{project_name}-mix-{default_branch}". It helps with readability.
There are more strings in the PR further down that should also be changed from .format() to f-strings.
brownie/project/main.py
Outdated
| msg += ( | ||
| "\n\nIf this issue persists, generate a Github API token and store" | ||
| " it as the environment variable `GITHUB_TOKEN`:\n" | ||
| "https://github.blog/2013-05-16-personal-api-tokens/" | ||
| ) |
There was a problem hiding this comment.
Avoid string concatenations with + (this can have unwanted side effects due to not being type safe). Instead use f-strings like msg = f"{msg}\n\nIf...".
I am fully aware that this is not done as rigorous as it should be throughout the code base and you can feel free to change any instances you come accross ;-).
brownie/project/main.py
Outdated
| "https://github.blog/2013-05-16-personal-api-tokens/" | ||
| ) | ||
| raise ConnectionError(msg) | ||
| elif r.json().get("default_branch") is None: |
There was a problem hiding this comment.
I would prefer elif "default_branch" not in r.json():
|
@matnad thanks for the remarks, I definitely agree with them as they help with readability down the line, and are in line with the style of the project. I implemented the corrections via d860bb5. |
There was a problem hiding this comment.
Thank you for your contribution! Overall I'm happy with the implementation, just a couple places where I'd echo @matnad 's comments re: preference for f-strings over .format. Once those are modified I'm happy to see this merge 🚀
Corrected by @iamdefinitelyahuman Co-authored-by: Ben Hauser <35276322+iamdefinitelyahuman@users.noreply.github.com>
Corrected by @iamdefinitelyahuman Co-authored-by: Ben Hauser <35276322+iamdefinitelyahuman@users.noreply.github.com>
Previously,
brownie bakewould assume the master branch was in eachbrownie-mix repository. Now it checks the github api to verify the
default branch first.
Resolves #915
What I did
MIXES_URLto now require 2 arguments when formatting the string, the project_name and default_branch_get_mix_default_branch(mix_name: str) -> str, which given the brownie-mix name, returns the default branch for the repositoryfrom_brownie_mixfunction to call_get_mix_default_branch, and include the returned value when formattingMIXES_URLRelated issue: #915
How I did it
I checked stackoverflow for how to find the default branch of a repository. Using the github api docs and
curl, I verified that endpoint worked and thatdefault_branchwas a key in the json returned. I then created the function_get_mix_default_branchto encapsulate the github api request, and then implemented it in thefrom_brownie_mixfunction.How to verify it
Currently, there are no brownie-mix repositories with a default branch not named
master. However, the implementation does not breaktests/project/test_brownie_mix.py.Checklist