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

Snom patch: header value syntax #258

Closed
wants to merge 5 commits into from

Conversation

pbertera
Copy link
Contributor

@pbertera pbertera commented Oct 1, 2016

Last header value: you can extract the header value using the ! suffix to the [last_headerName]

Example:
To: [last_From!]

will generates a To: header containing the last From header value, this is quite useful when you need to send a new request (Eg. re-INVITE into an inbound SIP dialog)

Of course you can do the same using a regexp, but this syntax simplifies a lot the scenario file.

All the attribution for this patch should go to Snom Technology AG

@pbertera pbertera changed the title Snom patch: header values syntax Snom patch: header value syntax Oct 1, 2016
@wdoekes
Copy link
Member

wdoekes commented Oct 2, 2016

Thanks for the patch with testcase.

Could you make that into [last_From.value] instead, as suggested in #132 ? The code changes you provided aren't as clean as they could be, but I'll wait with reviewing.

And, this lack all documentation, visible and invisible. A possible way to document its existence could be to add it to one of the built-in scenario's...

(Lastly, your commits aren't atomic changes. They'll need to be squashed before I could merge. And the testcase naming should either be github-#XXXX (reference to ticker XXXX on github) or some discriptive name, like last_header_value in which case the "github" makes no sense.)

@pbertera
Copy link
Contributor Author

pbertera commented Oct 3, 2016

Thanks for the feedback,

@wdoekes
Copy link
Member

wdoekes commented Oct 4, 2016

I would like to have an opinion about using : as a separator

Sounds good to me. (And it provides more legitimacy for the : being optional like it is right now.)

What about to add a new scenario (uas-req.xml) where the BYE comes from the uas?

I'm a bit reluctant to add new scenario's into the binary itself. If you add it to a samples directory as separate xml file, it would work for me. (Especially if a test case runs it, so the scenario's do not get stale.)

@wdoekes
Copy link
Member

wdoekes commented Oct 4, 2016

Closed in favor of WIP in #260.

@wdoekes wdoekes closed this Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants