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

Fix grammar, punctuation, spelling and some white-space #103

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

respencer
Copy link

I've cleaned up all the spelling and grammar mistakes I can find. I've also tried to fix punctuation mistakes, but I've been conservative.

I've started cleaning up white-space issues, but there's still a lot I can do.

@nodiscc
Copy link

nodiscc commented Aug 1, 2014

Sadly this doesn't merge properly in master anymore. I've tried merging it but there are simply too many changes. @respencer nice work, can you try applying this on current master (or better on https://github.com/shaarli/Shaarli master)?

Or should we drop this pull request because it's hard to merge? Shame, I reviewed the first 3 commits and they were perfect. Should have been merged earlier.

@respencer
Copy link
Author

I think you're going to have to reject this merge request and I'll do it again.

Anything you want left out of the new merge request?

@nodiscc
Copy link

nodiscc commented Aug 1, 2014

I'd leave the automatic HTML cleaner out for now. Current hand written code is dirty but it's maintainable. Plus HTML tidy seems to spew out large commits (difficult to review).

Other fixes (typos, css, id/class, whitespaces...) are welcome!

Currently it's to difficult to keep the changes and merge with upstream.
Conflicts:
	index.php
	tpl/linklist.html
	tpl/picwall.html
@respencer
Copy link
Author

Does that improve things @nodiscc?

@nodiscc
Copy link

nodiscc commented Aug 8, 2014

https://github.com/sebsauvage/Shaarli/pull/103/files looks better indeed.

Can you rebase all your changes on top of master, and squash your commits together (see http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)? There are 10 commits with 1000's of additions/deletions totaling to only 201 additions/ 205 deletions. This will hugely increase the git history size for no good reason.

I think this is a correct way to do it (you may have to change the remotes names):

git rebase sebsauvage/master
git rebase -i sebsauvage/master #edit/squash/reorder your commits as described above
git push respencer master

then respencer/master should be ahead of sebsauvage/master by only relevant commits (no undos, reverts, merge commits...). I suggest you open another pull request for this, to keep things clear.

@nodiscc
Copy link

nodiscc commented Aug 9, 2014

@respencer I can take care of the rebase if you want.

@respencer
Copy link
Author

I've already spent 2 days trying to get everything nice and neat, with the history intact. It's only really a few spelling and grammar fixes. Spending another day trying to figure out how to rebase, without screwing up the history, is in my opinion not an efficient use of time.

Squashing commits is for unpublished work. That horse bolted a year ago already.

Even that second link you provided says in the second paragraph that it's not a good idea for published work.

I'll rebase future commits.

@e2jk
Copy link

e2jk commented Aug 11, 2014

Published, meaning here "merged into the master branch".
The idea behind this request is to have a clean history in the master branch, allowing to better diagnose future issues, should any arise. If the investigation needs to jump through multiple commits, that add, then remove lines, it makes it unnecessarily complex, and would result in even less efficient use of time for the people involved.

@nodiscc please look at the rebase yourself then.

@respencer
Copy link
Author

@e2jk I understand the intention. I rebase regularly for work.

I think the pain is disproportionate to the gain in this instance.

I feel sorrow for any one trying to rebase this. They're going to suffer.

If I could figure out how to do it cleanly, I'd just start from scratch.

@nodiscc
Copy link

nodiscc commented Aug 11, 2014

Done, pull request at shaarli#20.
I've left out the id->class change for now, this deserves another pull request.
Thanks for your work @respencer, didn't mean to sound harsh; as this is a minor change I'd prefer not messing up the commit log.

@respencer
Copy link
Author

@nodiscc Thank you.

I didn't think you sounded harsh, so not a problem.

You may want to amend your commit. I assume you must have used a regex to speed things up. Unfortunately it changed to much.

'JPEG.' has been changed to 'JPe.g.' on line 2468 & 2485 of your index.php.

pikzen pushed a commit to pikzen/Shaarli that referenced this pull request Jan 27, 2015
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

3 participants