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

Acronym: New test case for apostrophe #1375

Closed
wants to merge 4 commits into from
Closed

Acronym: New test case for apostrophe #1375

wants to merge 4 commits into from

Conversation

alanvardy
Copy link
Contributor

The current solution for Acronym doesn't account for apostrophes, it is a more rare case for acronyms but an edge case nonetheless. Is this useful?

I added a single test case and incremented the version number.

Thank you!

The current solution in the Ruby path doesn't account for apostrophes, it is a more rare case for acronyms but an edge case nonetheless.
@rpottsoh rpottsoh changed the title New test case for Acronym Acronym: New test case for apostrophe Oct 21, 2018
Copy link
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

This looks like a good addition to me. However, please update version so it reads as "1.6.0" instead.

@alanvardy
Copy link
Contributor Author

Corrected to 1.6.0! Thank you.

@jpreese
Copy link
Contributor

jpreese commented Oct 21, 2018

My only concern with this would be that since there's already a case for a space and a case for a hyphen, adding a third case doesn't seem to bring a lot of value in my opinion.

Adding a second case forces the developer to do more than simply calling .split() with a single delimiter. Once you can do two, adding 3..4.. etc shouldn't be that different.

@alanvardy
Copy link
Contributor Author

I am bring this forward after an issue encountered on the Ruby track. When using the regex /\b\w/ or /\b[a-zA-Z]/ it would also grab the s at the end of Haley's etc. Because Ruby involves the use of scan() and regex rather than split() I think this could be useful.

@emcoding
Copy link
Contributor

emcoding commented Oct 21, 2018

Thanks @alanvardy for your suggestion!

However. Can we keep this on hold? We're in the process of rearranging the Ruby track, and adding Acronym as a core exercise is part of that. We're monitoring how it works. I'll add this issue to the evaluation.

That said, I agree with @jpreese that adding one extra case doesn't seem to bring much more value to the goal of the exercise. The extra case for the hyphen makes it clear that a simple split won't do. Another extra case of the apostroph seems a bit arbitrary - why not underscores, quotes and any other edge cases?

@alanvardy
Copy link
Contributor Author

Absolutely!

I perused through an online acronym dictionary and found plenty of hyphens, the occasional apostrophe and no underscores or quotes. I suppose I am thinking of what valid inputs are not covered, rather than being concerned with all inputs.

Nonetheless I thank you all for your time and the efforts you are all making! I will keep my fingers crossed for the review.

@emcoding
Copy link
Contributor

@rpottsoh Could this be interesting for other tracks?
If not, I'd rather close this PR. The issue is added to my evaluation notes of Acronym in Ruby, so we can reopen it if needed.
Is that okay with you?

@rpottsoh
Copy link
Member

@rpottsoh Could this be interesting for other tracks?

I think this is an interesting edge case to add. In other cases that contain punctuation, the punctuation is essentially "treated" like a space. The ' cannot be treated in this manner as it is not separating two words. For that reason I think it is worthwhile to test for apostrophe for the very reason I think @alanvardy is considering.

There is no reason to rush to merge this. If you would like to wait @F3PiX, that is fine. Just signal when ready.

If the group consensus is to close then I wont stand in the way. I'm just a participant here like the rest of you. 😄

@emcoding
Copy link
Contributor

Thanks all for your thoughts!
I'm closing this for now, and we can reopen it if need to.

@emcoding emcoding closed this Oct 22, 2018
@rpottsoh
Copy link
Member

rpottsoh commented Oct 22, 2018

reopening appears to not be possible. The repo that opened this PR has been deleted.

@alanvardy
Copy link
Contributor Author

This is my bad, when Github went down I was trying to create a new repository without success. I deleted a couple of repositories in case I was over a limit. One of those was this fork ;(. Wine and Github don't seem to mix.

How can I fix this?

@rpottsoh
Copy link
Member

I am not sure what your options might be. Restoring a deleted branch is documented here and in other places I am sure. Not sure how to restore a deleted repo.

In lieu of restoring your fork and restoring the ability to reopen this PR, you are probably faced with opening another PR.

@F3PiX closed this PR. I think it could have remained open until it was decided that the change to the test data was truly not desired. I don't fully understand why the activities of the Ruby track are of consequence to what happens in this repo. Tracks are allowed to update their exercise test suites and READMEs when they see fit to do so.

@emcoding
Copy link
Contributor

I don't mind reopening, but it's gone.

@rpottsoh
Copy link
Member

rpottsoh commented Oct 23, 2018

@alanvardy got a little too hasty the other day during the GitHub outage and ended up deleting his fork. He's more than welcome to open another PR. I think there is merit to the case he proposed.

@alanvardy
Copy link
Contributor Author

I'll submit another identical (and less transient!) PR later today and will reference in this thread. Thank you all for all your consideration!

@alanvardy
Copy link
Contributor Author

New PR created! #1377 .

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