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

Fix gender in passenger format #490

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

kim00425
Copy link

Extract gender from infant last name

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #490 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   94.06%   94.09%   +0.02%     
==========================================
  Files         120      120              
  Lines        2056     2066      +10     
==========================================
+ Hits         1934     1944      +10     
  Misses        122      122              
Impacted Files Coverage Δ
src/Services/Air/AirFormat.js 97.91% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 925754a...2cdeb50. Read the comment docs.

@kim00425
Copy link
Author

Extract gender from infant last name

@kim00425 kim00425 closed this Sep 18, 2020
@kim00425 kim00425 reopened this Sep 18, 2020
Copy link
Member

@dchertousov dchertousov left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

In case of non-INF Gender could be missing (as still could be parsed from name), but for the INF passengers you propose to always include gender based on First name.

IMO, we should to stick for the similar behaviour in all cases.

Also we try not to mutate function params, so I would like to see the code slightly modified:

  • if gender field exists, always take the gender from it
  • if gender field is absent, take the gender from the name
  • if name does not include strings like MR, RS, MSTR MISS etc, set gender to null
  • use separate variable when detecting gender, using rules above instead of modifying function params

Please also make sure, you have included all possible variants of MR/MRS strings and your reg exp is valid.

Current regex has some flaws:

(/MSTR|MISS$/gi).test('MSTRME/NAME')

this one will return true, while it should not. This would return false correctly:

(/(?:MSTR|MISS)$/gi).test('MSTRME/NAME')

@kim00425
Copy link
Author

Thanks for your PR!

In case of non-INF Gender could be missing (as still could be parsed from name), but for the INF passengers you propose to always include gender based on First name.

IMO, we should to stick for the similar behaviour in all cases.

Also we try not to mutate function params, so I would like to see the code slightly modified:

  • if gender field exists, always take the gender from it
  • if gender field is absent, take the gender from the name
  • if name does not include strings like MR, RS, MSTR MISS etc, set gender to null
  • use separate variable when detecting gender, using rules above instead of modifying function params

Please also make sure, you have included all possible variants of MR/MRS strings and your reg exp is valid.

Current regex has some flaws:

(/MSTR|MISS$/gi).test('MSTRME/NAME')

this one will return true, while it should not. This would return false correctly:

(/(?:MSTR|MISS)$/gi).test('MSTRME/NAME')

i did PR again check please

@sonarcloud
Copy link

sonarcloud bot commented Oct 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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