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

#134 new sprintf function allowing multiple key/value replacement #138

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Nunalves
Copy link

@Nunalves Nunalves commented Nov 12, 2016




/**
Copy link
Member

Choose a reason for hiding this comment

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

Good idea. I'll accept this function

end sprintf;


/**
Copy link
Member

Choose a reason for hiding this comment

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

Since all the code is in one PR I'll accept this but remove this function. It's already handled in standard sprintf

@martindsouza
Copy link
Member

@Nunalves I appreciate you submitting the code. I don't think we'll be implementing this for a few reasons:

  1. Requires that user has privilege to create types. In a lot of cases DBAs don't grant that.
  2. It's not really sprintf format as I'd like to keep with how it works
  3. We just implemented oos_util_string.multi_replace which may accomplish the same thing (just different syntax.

Let me know if you're ok with these reasons or if I'm missing something.

@Nunalves
Copy link
Author

@martindsouza i appreciate you getting back to me to check my opinion on this.
It is the same functionality in a different implementation, and it does require less permissions. But the comma delimited string is not the same as having an array of values. In the real world it will lead to a lot of concatenation happening in order to build the p_replace_str string with comma separated key/value pairs and manual concatenation is what we want to avoid in the first place by using the function.
I do prefer my implementation, i think it is easier to use. I'm already using mine heavily and happily in production :)

@martindsouza
Copy link
Member

@Nunalves all very good points and I hadn't considered the concatenation issue. Any idea on how to get around the schema level type option? This is one of my biggest concerns as I'd like to avoid any restrictions from developers using this.

One idea I have is what if we overload oos_util_string.multi_replace that takes in an array instead of a string. Something like this:

function multi_replace(
    p_str in varchar2,
    p_find_str in oos_util.tab_vc2,
    p_replace_str in oos_util.tab_vc2)
    return varchar2

This would work well when calling from PL/SQL ex:

oos_util_string.multi_replace('hello world', oos_util.tab_vc2('hello','world'), oos_util.tab_vc2('goodnight', 'moon');

The only negative thing is that you can't call it from SQL as it's not a recognized type.

Let me know what you think.

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