- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 461
 
make phonenumber plugin really a plugin #654
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
base: master
Are you sure you want to change the base?
make phonenumber plugin really a plugin #654
Conversation
          Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 
 Additional details and impacted files@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
- Coverage   97.84%   95.52%   -2.32%     
==========================================
  Files          78       78              
  Lines        3384     3352      -32     
  Branches      384      377       -7     
==========================================
- Hits         3311     3202     -109     
- Misses         42      119      +77     
  Partials       31       31              ☔ View full report in Codecov by Sentry.  | 
    
78c515d    to
    a574d57      
    Compare
  
    | 
           I had a quick look, looks very good overall. Generally I prefer having smaller commits for various reasons, notably as it's easier to spot regressions when they happen. Do you think it would be possible to split your commit in smaller ones, targeted on more granular changes?  | 
    
16ec9f5    to
    f12cc99      
    Compare
  
    3ea487f    to
    ebd1d78      
    Compare
  
    a8d295d    to
    320e6d5      
    Compare
  
    | 
           That's great, thanks so much. I'll review the individual patches ASAP.  | 
    
20107af    to
    98ec23c      
    Compare
  
    23137db    to
    1068146      
    Compare
  
    1068146    to
    b1a874c      
    Compare
  
    | 
           @claudep I think I'll do a PR adding some skips in tests that expect the phonenumber plugin to be present and perhaps refactor some of them before going on with this PR. What do you think?  | 
    
| 
           Feel free to make some more incremental PRs, even if it is for small things.  | 
    
Description
Motivation and Context
Fixes #469, fixes #581.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: