Skip to content

Pr-9#10

Open
CheezItMan wants to merge 2 commits intomasterfrom
Pr-9
Open

Pr-9#10
CheezItMan wants to merge 2 commits intomasterfrom
Pr-9

Conversation

@CheezItMan
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@aribray aribray left a comment

Choose a reason for hiding this comment

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

You should make sure all of these tests are passing

@aribray
Copy link
Copy Markdown

aribray commented Aug 15, 2019

Is it possible to have acronyms with special characters? Maybe the tests should account for those.

Comment thread acronym_test.rb
end

def test_lowercase_words
skip
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't forget to remove skip to make sure the tests are passing.

Copy link
Copy Markdown

@KateAnnNichols KateAnnNichols left a comment

Choose a reason for hiding this comment

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

Can you explain what the regex is targeting? Thanks!

Comment thread acronym_test.rb
# Common test data version: 1.7.0 cacf1f1
class AcronymTest < Minitest::Test
def test_basic
# skip
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can remove this commented out code.

Copy link
Copy Markdown

@JansenMartin JansenMartin left a comment

Choose a reason for hiding this comment

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

It seems like we're skipping most of these tests. Was that intentional?

Comment thread acronym_test.rb
def test_consecutive_delimiters
skip
assert_equal "SIMUFTA", Acronym.abbreviate('Something - I made up from thin air')
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What do we want to happen if the value is nil?

Comment thread acronym.rb
@@ -0,0 +1,5 @@
module Acronym
def self.abbreviate(value)
value.scan(/\b\w/).map(&:upcase).join
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we use a descriptive variable and an explicit return here?

Copy link
Copy Markdown

@goblineer goblineer left a comment

Choose a reason for hiding this comment

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

One of my instructors told me that submitting code with skipped tests would get me in big trouble. I won’t tell him if you don’t. 🤐

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.

7 participants