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

predefined pattern recognizer : IN_VEHICLE_REGISTRATION #1288

Merged
merged 33 commits into from
Feb 21, 2024

Conversation

devopam
Copy link
Contributor

@devopam devopam commented Feb 9, 2024

Change Description

Addition of a predefined recognizer : IN_VEHICLE_REGISTRATION

Describe your changes
Pattern, Context and Checksum based recognizer for India RTO issued vehicle registration number

Issue reference

This PR fixes issue #XX

Checklist

  • I have reviewed the contribution guidelines
  • I have signed the CLA (if required)
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required

devopam and others added 24 commits June 26, 2023 16:39
Added India PAN (Permanent Account Number) recognizer
refined the regex for better recognition and enhanced the test cases accordingly
Fixed lint error that was missed earlier.
Added test cases , verification and context data
Added negative test cases per review comments.
update pattern recognizer value per suggestion in review
added PresidioAnalyzerUtils class with generic functions. removed usage of stdnum
added test cases for analyzer_utils.py in prescribed format
added to the count of predefined recognizers
Added India specific predefined pattern recognizer for vehicle registration number
@devopam
Copy link
Contributor Author

devopam commented Feb 9, 2024

Hi @omri374 , please have a look when feasible and let me know your review comments

@omri374
Copy link
Contributor

omri374 commented Feb 9, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374
Copy link
Contributor

omri374 commented Feb 9, 2024

Thanks! I'll review it soon.

Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Thanks! great addition. Left a few comments

reinstated python 3.9 compatibility, reorganized code
@devopam
Copy link
Contributor Author

devopam commented Feb 12, 2024

Hi @omri374 ,
Please review when you get time to do so. I have incorporated changes to reflect on your review feedback to the best intent.
regards
Devopam

@omri374
Copy link
Contributor

omri374 commented Feb 15, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Logic reverted from analyzer_utils to recognizer classfile
added min size check to avoid failures per review comment
@devopam
Copy link
Contributor Author

devopam commented Feb 15, 2024

hi @omri374 ,
incorporated two comments and responded to another two. Please have a look when you get time , and let me know if this makes sense. And thank you for taking time out to go through it thoroughly. Much appreciated.

@omri374
Copy link
Contributor

omri374 commented Feb 18, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Thanks @devopam! I've left a few comments mainly around performance.

@microsoft microsoft deleted a comment from azure-pipelines bot Feb 20, 2024
@omri374
Copy link
Contributor

omri374 commented Feb 20, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@omri374 omri374 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 incorporating all the changes! Approved.

@omri374
Copy link
Contributor

omri374 commented Feb 21, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374 omri374 merged commit dee6562 into microsoft:main Feb 21, 2024
31 checks passed
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.

2 participants