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 methods that check for names in lists #867

Merged
merged 1 commit into from May 23, 2018

Conversation

Projects
None yet
4 participants
@jasonaowen
Contributor

jasonaowen commented Feb 26, 2018

Prefixes, given names, and suffixes can all have multiple entries, and so are kept in a list. The hasPrefix(String) and its sibling methods are conveniences for checking that these lists contain the given element. Add tests around those three methods to ensure that the list checking code is correct.

Issue #865 HumanName#hasGiven(String) can never return true

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Feb 26, 2018

@jamesagnew I noticed that the header said the file was generated, but I saw history in git and it wasn't obvious to me (at a glance) how it might have been generated. If I should instead fix the data that is used to generate this code, please point me in the right direction!

@grahamegrieve

This comment has been minimized.

Collaborator

grahamegrieve commented Feb 26, 2018

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Feb 26, 2018

@grahamegrieve ah okay. Shall I close this PR and leave the issue open, then?

@grahamegrieve

This comment has been minimized.

Collaborator

grahamegrieve commented Feb 26, 2018

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Feb 26, 2018

It does!

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Feb 26, 2018

I can rework it to just have @Ignored tests and no changes to the generated code, if you prefer. Or, if you want to point me to the generator, I'll take a look - let me know what would be most useful.

@grahamegrieve

This comment has been minimized.

Collaborator

grahamegrieve commented Feb 26, 2018

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Feb 26, 2018

@grahamegrieve thanks for fixing it so quickly!

I've updated this PR to just introduce unit tests. The tests are not currently @Ignored - let me know if you'd prefer to merge this PR before your fix, and then you can delete the @Ignore annotations when you make the tests pass.

@grahamegrieve

This comment has been minimized.

Collaborator

grahamegrieve commented Feb 26, 2018

Add unit tests for HumanName has(String) methods
Prefixes, given names, and suffixes can all have multiple entries, and
so are kept in a list. The `hasPrefix(String)` and its sibling methods
are conveniences for checking that these lists contain the given
element. Add tests around those three methods to ensure that the list
checking code is correct.

Issue #865 HumanName#hasGiven(String) can never return true
@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Feb 26, 2018

Done.

@coveralls

This comment has been minimized.

coveralls commented Feb 26, 2018

Coverage Status

Coverage decreased (-0.02%) to 72.925% when pulling 2f13964 on jasonaowen:master into 2eee606 on jamesagnew:master.

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Mar 13, 2018

@grahamegrieve Is there anything more I can do to help?

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Apr 19, 2018

Hi @grahamegrieve and @jamesagnew, is there any progress on the underlying issue? Is this still a useful pull request?

@grahamegrieve

This comment has been minimized.

Collaborator

grahamegrieve commented Apr 19, 2018

@jasonaowen

This comment has been minimized.

Contributor

jasonaowen commented Apr 19, 2018

@grahamegrieve great, thank you!

@jamesagnew jamesagnew merged commit 6da4ec1 into jamesagnew:master May 23, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
coverage/coveralls Coverage decreased (-0.02%) to 72.925%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jamesagnew added a commit that referenced this pull request May 23, 2018

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented May 23, 2018

Tests have been merged and verified working now. Thanks @jasonaowen and @grahamegrieve !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment