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

docs: Corrected usage of pipenv sync Update READEME.md #12707

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xwitty
Copy link
Contributor

@0xwitty 0xwitty commented Jan 9, 2025

I noticed a issue in the documentation regarding the use of the pipenv sync command. The command is typically run without the -m flag, as pipenv is executed directly rather than as a module through python3.

This change updates the relevant section to remove python3 -m before pipenv, ensuring the documentation reflects the correct usage and avoids potential confusion for users.

@0xwitty 0xwitty requested a review from a team as a code owner January 9, 2025 08:26
@0xwitty 0xwitty requested a review from wacban January 9, 2025 08:26
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.64%. Comparing base (5b56156) to head (78c459a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12707   +/-   ##
=======================================
  Coverage   70.64%   70.64%           
=======================================
  Files         848      848           
  Lines      173693   173693           
  Branches   173693   173693           
=======================================
+ Hits       122707   122713    +6     
+ Misses      45856    45849    -7     
- Partials     5130     5131    +1     
Flag Coverage Δ
backward-compatibility 0.16% <ø> (ø)
db-migration 0.16% <ø> (ø)
genesis-check 1.36% <ø> (ø)
linux 69.24% <ø> (+<0.01%) ⬆️
linux-nightly 70.25% <ø> (-0.01%) ⬇️
pytests 1.66% <ø> (ø)
sanity-checks 1.47% <ø> (ø)
unittests 70.48% <ø> (+<0.01%) ⬆️
upgradability 0.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

Sorry but I'm not familiar with the conventions around python / pipenv. Please seek other reviewer if this change is needed for your project.

From my point of view:
There are plenty of other usages of pipenv as a module across this file so this PR breaks this convention. The old command seems to work so I'm not sure if this change is worth it.

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.

2 participants