Skip to content

Conversation

@breyana
Copy link
Collaborator

@breyana breyana commented Mar 1, 2017

No description provided.

var Bob = function() {};

Bob.prototype.hey = function(what) {
if (/([A-Z]+\s){2,}|[A-Z](?=!)/.test(what)) {

Choose a reason for hiding this comment

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

Good use of RegExp, especially the lookahead 👍

var result = bob.hey('\xdcML\xc4\xdcTS!');

var result = bob.hey('\xdcML\xc4\xdcTS!');

Choose a reason for hiding this comment

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

I'm curious what the intention of this test and the special characters test above it are. Both of those tests have ordinary capitalized letters followed by exclamation marks, which should match the RegExp regardless of whether the special characters match.

I know you didn't write it, I'm just not sure that it's testing anything different than the other cases and wanted to point that out. 😋

var SecretHandshake = function(input) {
if (typeof input !== 'number') {
throw new Error('Handshake must be a number')
}

Choose a reason for hiding this comment

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

👍

var bracket = function(input) {
//define what ending bracket should be if the beginning is found
};

Choose a reason for hiding this comment

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

You should try to keep incomplete placeholders like this out of commits for complete tests, that way you have more freedom to cherry pick commits later or reorder them during a rebase without ending up with files getting reverted to a broken state.

Personally I try to make a separate commit for each change (ex: secret handshake would be a different commit from bob, and bob's commit would never have changes to secret handshake's files) but that's just one way of keeping git stuff organized.

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.

3 participants