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

Add warn about requirements.txt file to "requirements without setup.py" section of readme #959

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

hramezani
Copy link
Member

@hramezani hramezani commented Oct 21, 2019

Resolves #744.

Changelog-friendly one-liner: Add warn about requirements.txt file to "requirements without setup.py" section of readme.

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@atugushev atugushev requested a review from tysonclugg October 21, 2019 19:29
@atugushev
Copy link
Member

Hello @tysonclugg,

Sorry for bothering you. I've requested a review from you since you are the author of the changed paragraph. Could you please review it when you have time? Thanks!

@codecov
Copy link

codecov bot commented Oct 21, 2019

Codecov Report

Merging #959 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #959   +/-   ##
======================================
  Coverage    99.1%   99.1%           
======================================
  Files          34      34           
  Lines        2358    2358           
  Branches      305     305           
======================================
  Hits         2337    2337           
  Misses         11      11           
  Partials       10      10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef52977...aacd085. Read the comment docs.

README.rst Outdated Show resolved Hide resolved
@atugushev atugushev added the docs Documentation related label Oct 21, 2019
@atugushev atugushev changed the title Add warn about requirements.txt file to "requirements without setup.py" section of readme. Add warn about requirements.txt file to "requirements without setup.py" section of readme Oct 22, 2019
@hramezani
Copy link
Member Author

Isn't it ready to merge?

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not a native English speaker, so better wait for approvement from another member.

@atugushev atugushev requested a review from vphilippon October 24, 2019 14:36
@tysonclugg
Copy link
Contributor

The warning is reasonable, but it applies equally if building requirements.txt from either setup.py or requirements.in. The best solution may be to include a similar warning in a opening statement about what pip-sync and other tools are supposed to achieve, rather than repeating the same warning in multiple sections.

@hramezani hramezani force-pushed the issue_744 branch 2 times, most recently from aca27f6 to 35c1d0d Compare October 26, 2019 22:13
@hramezani
Copy link
Member Author

@tysonclugg Thanks,
@atugushev I moved it as a note to the top of the readme file.

@hramezani hramezani force-pushed the issue_744 branch 2 times, most recently from 035a2ed to 63d0ea4 Compare October 26, 2019 22:25
@atugushev atugushev added this to the 4.3.0 milestone Oct 28, 2019
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Markup looks broken, see the how it's rendering:
image


Besides, I think the note should be added to the "Example usage for pip-compile" section. Probably, better to emphasize that the issue with the existing requirements.txt causes only if you start from scratch. I would suggest:

Note: ensure you don't have requirements.txt if you compile setup.py or requirements.in from scratch, otherwise, it might interfere.

@atugushev
Copy link
Member

@tysonclugg good point! Thanks :)

@atugushev atugushev self-requested a review November 21, 2019 06:11
@hramezani
Copy link
Member Author

@atugushev it's done.

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thanks!

@atugushev atugushev merged commit 8508b37 into jazzband:master Nov 22, 2019
@atugushev
Copy link
Member

atugushev commented Nov 22, 2019

@tysonclugg and @mbeacom, thanks for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readme: "requirements without setup.py" section doesn't warn that it considers existing requirements.txt file
4 participants