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

Editing/Reviewing #95

Merged
merged 5 commits into from
Mar 22, 2018
Merged

Editing/Reviewing #95

merged 5 commits into from
Mar 22, 2018

Conversation

luizperes
Copy link
Contributor

@luizperes luizperes commented Mar 21, 2018

This PR brushes up the files CONTRIBUTING and README. More edits will be done later :)

This closes #93

@codecov-io
Copy link

codecov-io commented Mar 21, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #95   +/-   ##
======================================
  Coverage    83.7%   83.7%           
======================================
  Files           8       8           
  Lines         362     362           
======================================
  Hits          303     303           
  Misses         59      59

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 7768525...b95e5bb. Read the comment docs.

@ryukinix ryukinix self-requested a review March 21, 2018 05:07
@ryukinix ryukinix added this to the v0.4.0 milestone Mar 21, 2018
@ryukinix
Copy link
Owner

Thank you for this, @luizperes! This indeed helps.

Copy link
Owner

@ryukinix ryukinix left a comment

Choose a reason for hiding this comment

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

This is a nice review of docs, thanks. Really you made it more pleasant and readable. I have only one problem, can you change make install to pip install --user .? I know that this assumes that users should have pip, however, make install try install globally which needs super-user permissions... this is bad and doesn't reflects well the text written.


Also feel free to open new issues for any bug you found, features you think would be nice to have or questions in general.
- 1. Find an issue [here](https://github.com/ryukinix/mal/issues).
Copy link
Owner

Choose a reason for hiding this comment

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

This is awesome. Really great. Algorithmic steps makes thing more clear

- 3. Mention (by commenting on the issue) that you want to take it.
- 4. Fork it and create a new branch out of `master`.
- 5. Work on it.
- 6. Submit a pull request.
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, please, submit PRs, do not die on the cold planet of ideas.


For more information see [Development Mode](http://setuptools.readthedocs.io/en/latest/setuptools.html#development-mode).

# Running without need to install
# Running mal without installing it
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, this really makes things more clear.

README.md Outdated

```
make install
```

Probably we'll need have super-user permissions, but I'd recommend you
to install inside of a virtualenv or use the `pip install --user' stuff.
It is likely that `mal` will never need super-user permissions, but we recommend its
Copy link
Owner

Choose a reason for hiding this comment

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

Actually this is a little misleading, because make install needs super-user permissions if we call inside of a virtualenv.
Probably the most correct way to fix all this thing is just point the manual installation as pip install --user . which will build the package and install in the userspace /home/$USER.

If you're using the archlinux distro this project has been packaged and uploaded to
the AUR as [python-mal-git](https://aur.archlinux.org/packages/python-mal-git).
This project has been packaged and uploaded to the AUR as
[python-mal-git](https://aur.archlinux.org/packages/python-mal-git) in case you're using an archlinux distro.
Copy link
Owner

Choose a reason for hiding this comment

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

Indeed better


### Using The Interface

When `mal` is executed without any arguments the help message is displayed:
When `mal` is executed without any arguments, a help message is displayed:
Copy link
Owner

Choose a reason for hiding this comment

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

indeed...

@luizperes
Copy link
Contributor Author

I have changed the points you mentioned. :) I believe it is much better now, please let me know what you think!

Copy link
Owner

@ryukinix ryukinix left a comment

Choose a reason for hiding this comment

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

Almost there.

README.md Outdated
This project has been packaged and uploaded to the AUR as
[python-mal-git](https://aur.archlinux.org/packages/python-mal-git) in case you're using an archlinux distro.

You may install it using an AUR wrappers such `yaourt` or `pacaur`, making
Copy link
Owner

@ryukinix ryukinix Mar 21, 2018

Choose a reason for hiding this comment

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

Please, rollback that section about Arch Linux without details, I already remove that in earlier version. As I said for you, README.md was the doc more updated, I forgot to remove that from sphinx docs (please, remove from that too)

README.md Outdated
@@ -60,27 +60,54 @@ pip install --user mal

### Manual Installation

Clone this project and run inside it:
Clone this project and inside it run:

```
make install
Copy link
Owner

Choose a reason for hiding this comment

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

You miss that part that I said before? Please, remove that I write this instead: pip install --user .

@luizperes
Copy link
Contributor Author

That sounds good! Will change the it later tonight :)

@luizperes
Copy link
Contributor Author

Have just done it. Please let me know what you think @ryukinix


```
make install
pip install --user .
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@ryukinix
Copy link
Owner

Thanks for this.

@ryukinix ryukinix merged commit fa9024d into ryukinix:master Mar 22, 2018
@bradenbest
Copy link
Contributor

bradenbest commented Mar 22, 2018

Tell you what, to make up for all the pestering, I'll start working on my own proofreading pull request some time later today. Looking over the docs, I've already noticed some miscellaneous typos chiefly in the grammar and sentence flow departments. As a native English-speaker and friendly neighborhood grammar nazi, I'll put the docs through the same OCD edit re-edit proofreading rigor that I put my YouTube comments through, but without making it 18 paragraphs long!

@bradenbest
Copy link
Contributor

@ryukinix Quick fact check: this is actually a fork of pushrax/mal and not from scratch, correct? It doesn't say so on the project page (where it usually says "forked from xxxx"), so I want to be sure.

Original: This project was initially inspired in pushrax/mal.
Planned edit: This project is a fork of pushrax/mal, which seems to have fallen out of maintenance.

@ryukinix
Copy link
Owner

ryukinix commented Mar 22, 2018

@bradenbest
In some way, yes, we can look this as a some type of fork, but not a GitHub fork however.

I indeed based most of the initial features on pushrax/mal, as the API and inc/dec stuff, but we have uncorrelated git histories, which can be confusing saying that is a fork on README.md. I based this works on pushrax/mal mainly because I was needing a mal CLI software and all what I found was pushrax/mal (and because that I'm referring about him on README.md until today).

I think we have more important stuff to take care and the way this fact is presented on README it's good enough describing what happened.

@bradenbest
Copy link
Contributor

Got it, I'll say it's an "unofficial fork" to prevent people from thinking it's a direct GitHub fork.

@bradenbest
Copy link
Contributor

Also, the README is lookin' pretty good. When I finish it, I'll submit a pull request and start working on the other docs

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

Successfully merging this pull request may close these issues.

Update sphinx docs at /docs before official 0.4.0 release
4 participants