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

Remove repeated string. #1663

Merged
merged 2 commits into from
Apr 25, 2014
Merged

Remove repeated string. #1663

merged 2 commits into from
Apr 25, 2014

Conversation

rparrapy
Copy link
Contributor

Fixes minor documentation error.

@@ -10,7 +10,7 @@ pass for your copy of CKAN. This section explains how to run CKAN's tests.
-------------------------------
Install additional dependencies
-------------------------------

r
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? :P

@rparrapy
Copy link
Contributor Author

Fixed. Sorry about that.

If you are have the ``ckan-migration`` option on the tests will reset the
reset the database before the test run.
If you have the ``ckan-migration`` option on the tests will reset
the database before the test run.
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're working on this issue, could you reword this phrase? It still reads very odd. Maybe something like

If you enable the ``--ckan-migration`` option, the database will be reset before running the tests. This is useful to test migrations.

Or something like that. What do you think?

@vitorbaptista vitorbaptista self-assigned this Apr 23, 2014
@rparrapy
Copy link
Contributor Author

You are right, your sentence is much clearer.
I didn't change a word, since I'm very new to CKAN and nosetests. Just wanted to add my 2 cents with english :)
Fixed another typo in the last commit.

@vitorbaptista
Copy link
Contributor

@rparrapy I took a closer look at that page's docs and I realize now that that phrase (If you enable...) is a bit redundant. We're already explaining that feature right after, under the Migration testing section. So, IMO, we could simply remove that phrase entirely.

What do you think?

If you agree, please go ahead and do it and I'll merge this. As another note, could you make your commit messages a bit clearer and follow the pattern in our how to write commit messages page of our contributing guide? Also add the [#1663] issue number to the first commit message's line, and remember to put a linebreak between the first line and the rest of the commit message (if there's any).

I'm sorry I'm being a bit pedantic on this pull request. I really appreciate that you've taken the time to work on it. It's just that the docs are really important, and I want them to be as good as possible. Thanks a lot for your help (and patience 😄)

@vitorbaptista
Copy link
Contributor

P.S.: It's also good to, when editing these things, keep the line length below 80 chars 👍

@rparrapy
Copy link
Contributor Author

@vitorbaptista I thought it was kind of redundant, but I wasn't quite sure since I'm fairly new to CKAN.

Don't worry about the corrections, I am awared I didn't follow all the guidelines for contributions.
In my defense, I must say that some of the mistakes were caused by editing the file through the github editor (I know, I'm lazy). For example, the branch name was set automatically and I think the lack of a blank line between commit phrases was also github's fault :)

I guess I'll have to make it properly now. I'll try to correct the commit messages, though I must warn you I'm kind of a git noob too. I read it can be done with an interactive rebase, do you think that's the best way to do it?

Anyway, I'll keep your suggestions in mind since I intend to keep contributing to CKAN, if possible.
Thanks for your time.

@vitorbaptista
Copy link
Contributor

@rparrapy There's no problem at all! I know we have quite a bit of guidelines. The good thing is that you'll already know how to do it next time 😄

About the commits, you could use rebase/squash (tutorial in http://git-scm.com/book/en/Git-Tools-Rewriting-History), but it's a quite complicated thing to do for the first time (but it's a really useful git feature to know).

To make it simple, what I would do if I were you would be something like:

git checkout patch-1    # to change to your branch
git reset --hard master # this will remove every commit that you created on it that isn't on master
# do the new changes
git commit
git push --force

This usually isn't good, because what you're doing here is rewriting history: if someone else was working on this same branch, you would end up creating a mess for them (we would never do it on the main CKAN repository, for example). But as it's in your personal fork, and in another branch, it's OK in my opinion.

In the end, you'll have just a single commit removing the line and fixing that typo (or two if you prefer).

@rparrapy
Copy link
Contributor Author

@vitorbaptista is it better now?

I have one more (perhaps silly) question.
I am supposed to add the issue_id to the commit message (i.e. [#1663]). However, if I'm not working on a previously registered bug, I get the issue_id just after creating the pull request. This means, obviously, that I didn't know the issue_id by the time I created my branch or commited the changes to my fork.

My question is: am I supposed to --amend always in this case? or is there some other workflow that I don't know about?

Again, thanks a lot for your help.

@vitorbaptista
Copy link
Contributor

@rparrapy That's perfect! Merged 👍

The question isn't silly at all. In that case, you wouldn't add the issue number. But unless you're working on something trivial (like fixing a typo), it's almost always better to create an issue before starting working on it. This gives a place to talk about the issue (there may be other people interested/affected by it), and allow you to ask any questions.

If you have any other doubts or want to chat, we also hang out by the ckan-dev mailing list, and at #ckan in Freenode :)

Congratulations for your first merged pull request!

Cheers! 🍻

@vitorbaptista vitorbaptista reopened this Apr 25, 2014
vitorbaptista added a commit that referenced this pull request Apr 25, 2014
@vitorbaptista vitorbaptista merged commit ecdafe3 into ckan:master Apr 25, 2014
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.

None yet

2 participants