Skip to content

Further phone and date RegEx corrections. (TRAVIS: passed) (LINT: passed) #514

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

Closed
wants to merge 16 commits into from

Conversation

g1smd
Copy link
Contributor

@g1smd g1smd commented Sep 16, 2012

Updated and refactored GB phone RegEx pattern.
Fix holes in date validations.
Standardise comment format.
Updated reference URL.
Fix travis/lint errors.

@g1smd
Copy link
Contributor Author

g1smd commented Sep 16, 2012

Some overlap with 505 and 510.

@Synchro
Copy link
Contributor

Synchro commented Sep 17, 2012

I'm not sure what's going on here - you're reposting some changes from my fork in #510 in this pull request! If you backed out 3cc98ea and 3cc98ea, your changes would stand alone with no overlap.

While I'm here, how about some test cases for your phone number changes?

@g1smd
Copy link
Contributor Author

g1smd commented Sep 17, 2012

I thought I had your stuff in a separate branch. I'll also take a look at the tests.

@Synchro
Copy link
Contributor

Synchro commented Sep 17, 2012

No problem - I just noticed that GitHub doesn't let you fork a repo if you already have a fork of its parent.

@Synchro
Copy link
Contributor

Synchro commented Nov 6, 2012

I'm not sure why you're saying it's tested - I don't see any new tests for your changes, i.e. to prevent regression to the previous cases that passed when they should not have. I don't see why you backed out the changes in 032e9e8.
Also this is still treading all over my pull request which you copied.

@g1smd
Copy link
Contributor Author

g1smd commented Nov 6, 2012

Merged. Updated. Tested (TRAVIS: passed; LINT: passed). Ready to Commit.

@Synchro
Copy link
Contributor

Synchro commented Nov 7, 2012

I can't see your fork in Travis-ci, and the 'details' link above doesn't point at anything useful. Have I missed something?

@g1smd
Copy link
Contributor Author

g1smd commented Nov 10, 2012

Have I missed something?

Yes, look at the red and green dots over to the right side of the box, but only on recent commits.

If you merge your repo with the master, you'll get your first personalised Travis report this way too.

@g1smd
Copy link
Contributor Author

g1smd commented Nov 12, 2012

Merged. Updated. Tested (TRAVIS: passed; LINT: passed). Ready to Commit.

@jzaefferer
Copy link
Collaborator

Does this still overlap with #510? Would be good to merge that separately. Also an interactive rebase to squash most of the fixup commits would help. Btw. what are all the whitespace changes in test files about?

@g1smd
Copy link
Contributor Author

g1smd commented Nov 23, 2012

It does overlap with 510 as they are proposing something slightly different (and I guess 510 will throw errors in Travis).

The whitespace didn't seem consistent, tried to make it more so. Code alignment also helps readability.

@jzaefferer
Copy link
Collaborator

Travis just runs grunt, so let's refer to that. Can you do the rebase and squashing?

@g1smd
Copy link
Contributor Author

g1smd commented Nov 23, 2012

Last time I tried rebase, it completely messed up. I'm a bit wary of it now.

@jzaefferer
Copy link
Collaborator

Try git rebase master -i and follow the instructions in the editor. When done, check the log to see if it all looks good, before you force-push back to GitHub.

@g1smd
Copy link
Contributor Author

g1smd commented Nov 23, 2012

I'm not using Linux.

@jzaefferer
Copy link
Collaborator

Its the same command on every system, its just git. If you're using a GUI tool on Windows, install msysgit.

@g1smd
Copy link
Contributor Author

g1smd commented Nov 23, 2012

I don't think it will help matters:

One of the vast uses of git rebase -i is reordering commits.Do NOT rebase commits that have been pushed publicly. If you’re working with other developers, that will make the merge non fast-forwardable and creates headaches for all involved. Yes, the command allows one to rewrite history, but only do so when the commits are still on your local machine.

@Synchro
Copy link
Contributor

Synchro commented Nov 23, 2012

Really, your repo is really a private one that happens to be publicly visible - nobody has made any forks from it, so don't worry about it, go ahead and do the rebase.

@jzaefferer
Copy link
Collaborator

Yep, I wouldn't have asked for a rebase otherwise. That warning very much applies to commits pushed to this repo's master branch. But not to other branches or even forks.

@g1smd
Copy link
Contributor Author

g1smd commented Nov 23, 2012

-> "Nothing needs rebase; master equal remote/origin/master".

@Synchro
Copy link
Contributor

Synchro commented Nov 23, 2012

You're missing the point - rebasing will convert the 16 commits in this pull request into 1, making for a much tidier merge upstream. The result is the same, it's just less messy.

@nschonni
Copy link
Collaborator

I think @jzaefferer was talking about this kind of rebase http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html not just playing back your commits onto of the remote master.

@jzaefferer
Copy link
Collaborator

Rebased onto master, squashing into a single commit and fixing a few lint issues.

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