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

Updated README #31

Merged
merged 2 commits into from
Feb 23, 2015
Merged

Updated README #31

merged 2 commits into from
Feb 23, 2015

Conversation

jturnbull
Copy link
Contributor

Added better description.

@meshy please review

Added better description.
@jturnbull
Copy link
Contributor Author

@meshy reverted my master commits

@meshy
Copy link
Collaborator

meshy commented Feb 23, 2015

@jturnbull there two other commits that did not get reverted by the looks of it.

@meshy
Copy link
Collaborator

meshy commented Feb 23, 2015

...and it would have been better to PR the fix for setup.py, rather than push the revert commit to master

@jturnbull
Copy link
Contributor Author

@meshy all three commits were reverted, despite the comment.

...and it would have been better to PR the fix for setup.py, rather than push the revert commit to master

Why? This way you can review all the changes in one place

@meshy
Copy link
Collaborator

meshy commented Feb 23, 2015

Why?

So that the code change can be reviewed. There was no need to add a revert. The change was fundamentally sound, but it introduced a bug. Better to fix the bug.

@LilyFoote
Copy link
Collaborator

I thought PyPI doesn't support markdown?

@meshy
Copy link
Collaborator

meshy commented Feb 23, 2015

Also, if you revert more than one commit together in a bunch again, please add the commit hashes to the commit message.

@meshy
Copy link
Collaborator

meshy commented Feb 23, 2015

@Ian-Foote it doesn't.

url='https://github.com/incuna/django-orderable',
long_description=open('README.rst').read(),
long_description=open('README.md').read(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to drop the long_description, or should we continue to use rst? Or is there another solution?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should continue to provide a markdown long description.

@codecov-io
Copy link

Current Coverage via 24cca87 is 68.91%

Merging readmeupdates into master will change coverage by -1.68%

codecov.io

@@            master   readmeupdates   diff @@
==============================================
  Files            2               2       
  Stmts          119             119       
  Branches         0               0       
==============================================
- Hit             84              82     -2
  Partial          0               0       
- Missed          35              37     +2

@jturnbull
Copy link
Contributor Author

@meshy should I try to up the coverage in this PR? ;)

@meshy
Copy link
Collaborator

meshy commented Feb 23, 2015

Haha! I think we can let that one slip ;)

meshy added a commit that referenced this pull request Feb 23, 2015
@meshy meshy merged commit 3e6151a into master Feb 23, 2015
@meshy meshy deleted the readmeupdates branch February 23, 2015 22:45
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.

4 participants