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

nathelper: add optional set_contact_alias([trim]) parameter #2388

Merged
merged 1 commit into from Jul 3, 2020

Conversation

eschmidbauer
Copy link
Contributor

@eschmidbauer eschmidbauer commented Jul 2, 2020

Pre-Submission Checklist

  • Commit message has the format required by CONTRIBUTING guide
  • Commits are split per component (core, individual modules, libs, utils, ...)
  • Each component has a single commit (if not, squash them into one commit)
  • No commits to README files for modules (changes must be done to docbook files
    in doc/ subfolder, the README file is autogenerated)

Type Of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist:

Description

add support for trimming an already existing alias parameter when calling set_contact_alias()

@eschmidbauer eschmidbauer requested a review from miconda July 2, 2020 15:47
@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2020

This pull request introduces 1 alert when merging f254478 into 798c1c6 - view on LGTM.com

new alerts:

  • 1 for Local variable hides global variable

@miconda
Copy link
Member

miconda commented Jul 3, 2020

Thanks!

If nobody has anything against, this can be merged before branching 5.4 to fix the situation of "strange" UA that take the alias from 200ok of REGISTER and use it for subsequent requests. It is an optional function parameter, not changing the existing behaviour when not set.

@eschmidbauer eschmidbauer merged commit 07f2857 into master Jul 3, 2020
@eschmidbauer eschmidbauer deleted the nathelper-set_contact_alias-trim branch July 3, 2020 14:32
@miconda
Copy link
Member

miconda commented Jul 3, 2020

@eschmidbauer - my comment was to wait a few days before merging to see the opinion of other developers, not to merge. Master is frozen now for new features.

@miconda
Copy link
Member

miconda commented Jul 3, 2020

There is also an issue reported by LGTM scanner: 'Local variable hides global variable', see above comments from their bot.

@eschmidbauer
Copy link
Contributor Author

Apologies, I misunderstood. I can revert the commit

@eschmidbauer
Copy link
Contributor Author

reverted and resubmitted PR (#2390) with update to address the LGTM scanner issue

@miconda
Copy link
Member

miconda commented Jul 3, 2020

A fair amount of time should be allowed for comments, considering time zones, traveling, etc ...

With the revert, I noticed that a new function was actually added to the core. If wanted there, then it should have been a separate commit for it. We want commits per components, so maintainers of each component can see easier their code is affected.

@eschmidbauer
Copy link
Contributor Author

ive updated the new PR with 2 separate commits, one for core and one for the module

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.

None yet

2 participants