Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

chore: Readme clarity improvements #1271

Merged

Conversation

RostarMarek
Copy link
Contributor

Added a few parts to readme files to incentivize using Docker setup instead of local environment. Also made sure these aprts link to dockerize setup in Build.md.
Once I have a better understanding of simulator and scripts I will also probably try to write up some at least small guide for those.

Copy link
Collaborator

@NickeZ NickeZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Collaborator

@benma benma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. I left a a nit about the wording for Python.

Comment on lines 26 to 27
such as regenerating files have their libraries NOT in requirements.txt as they are not always necessary to run python
scripts.
Copy link
Collaborator

@benma benma Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

not always necessary to run the python scripts => not necessary for the bitbox02 Python package.

They are never necessary for the bitbox02 package, as these are dev-dependencies not relevant for the package users.

@RostarMarek RostarMarek requested a review from benma August 15, 2024 13:28
@benma
Copy link
Collaborator

benma commented Aug 27, 2024

Please rebase, there appears to be a conflict already 😇

@RostarMarek RostarMarek force-pushed the rostarmarek/readme-suggestions-vol1 branch from a5053c6 to 808e6cf Compare August 27, 2024 12:48
Copy link
Collaborator

@benma benma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Please squash so there is one commit instead of two.

@RostarMarek RostarMarek force-pushed the rostarmarek/readme-suggestions-vol1 branch from 808e6cf to 89805cd Compare August 27, 2024 13:12
@RostarMarek RostarMarek requested a review from benma August 27, 2024 13:13
BUILD.md Outdated
`make dockerdev` expecting specific version of the image.

```sh
docker pull shiftcrypto/firmware_v2:latest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So sorry for missing this 😇, but since last week we don't use :latest but the version in .containerversion. See https://github.com/BitBoxSwiss/bitbox02-firmware/pull/1282/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R134

Please change this line to make dockerpull.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will amend the commit so it stays as 1 in a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be up to date

Reword the python readme based on suggestion from comments.

Signed-off-by: RostarMarek <rostarmarek@gmail.com>

chore: Readme clarity improvements

Added a few parts to readme files to incentivize using
Docker setup instead of local environment.

Signed-off-by: RostarMarek <rostarmarek@gmail.com>
@RostarMarek RostarMarek force-pushed the rostarmarek/readme-suggestions-vol1 branch from 89805cd to fa6c437 Compare August 27, 2024 13:24
@RostarMarek RostarMarek requested a review from benma August 27, 2024 13:24
@benma benma merged commit 94efe7b into BitBoxSwiss:master Aug 27, 2024
@RostarMarek RostarMarek deleted the rostarmarek/readme-suggestions-vol1 branch August 27, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants