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

core: added xavu_serialize_fields function #2603

Merged
merged 2 commits into from Jan 18, 2021

Conversation

nchaigne
Copy link
Contributor

Also added a function called by the three "*_serialize_fields" to reduce code duplication.

The interface is not modified.

Type Of Change

  • New feature (non-breaking change which adds new functionality)

Checklist:

  • Tested changes locally

Description

Small pull request that adds a function "xavu_serialize_fields" (with same interface as "xavp_serialize_fields" and "xavi_serialize_fields"), and an internal helper function to reduce code duplication.

Also added a function called by the three "*_serialize_fields" to reduce code duplication.

The interface is not modified.
@miconda
Copy link
Member

miconda commented Jan 18, 2021

Actually the duplicate of code in this case was intentional, because the code for xavp, xavi and xavu should stay independent of each other, they may diverge more in the future and a change in one can break the others.

There are a lot of functions that are somehow duplicate for the three types of xavps, when it becomes clear that they are going to stay very tight, then many functions may be combined, but till then I prefer to keep everything specific for one type. Also, when an eventual merging is decided, we have to balance the complexity of adding many IF conditions (to detect the type, then to figure out comparison of the names to be case sensitive or not, a.s.o.) vs some code duplication but clearer api to maintain.

You can make the pull request that just adds the independent xavu_serialize_fields() function and it will be merged.

@nchaigne
Copy link
Contributor Author

Thanks for your feedback.
I'll amend this pull request with only the xavu function then.

We can add xavu_serialize_fields, but do not change the other functions.
@miconda
Copy link
Member

miconda commented Jan 18, 2021

Thanks!

@miconda miconda merged commit f9261f1 into kamailio:master Jan 18, 2021
NGSegovia pushed a commit to NGSegovia/kamailio that referenced this pull request Aug 26, 2021
* core: added xavu_serialize_fields function

Also added a function called by the three "*_serialize_fields" to reduce code duplication.

The interface is not modified.

* Revert refactoring

We can add xavu_serialize_fields, but do not change the other functions.
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