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

Allow parameter name patterns to be varied from and restored to default values. #622

Merged
merged 1 commit into from
Feb 21, 2018

Conversation

MMatten
Copy link
Contributor

@MMatten MMatten commented Feb 6, 2018

Allow parameter name pattern to be varied from and restored to the default name pattern string for each adapter via a new Parameter Pattern option.


public Pattern getParameterPattern() {
return paramsNames;
return Pattern.compile(getParamPattern());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace issue here noted.

@MMatten MMatten force-pushed the allow-variable-param-name-patterns branch from daadd68 to 3652629 Compare February 6, 2018 20:31
@javornikolov
Copy link
Contributor

@MMatten I think something that should be OK.

Just we rather have diffent naming since it's hard to guess what's the difference between getParameterPattern and getParamPattern.

@MMatten
Copy link
Contributor Author

MMatten commented Feb 6, 2018

Just we rather have diffent naming since it's hard to guess what's the difference between getParameterPattern and getParamPattern.

Very true. I was already thinking of that too.

How about public Pattern getParameterPattern() and private getParamPatternString()?

@javornikolov
Copy link
Contributor

javornikolov commented Feb 7, 2018

It's still a bit confusing. The distinguishing part I think is that one of these is the default pattern (for the environment), isn't it?

Edit: above comment was a bit down the road and before I look at the current implementation. For the current implementation it might be OK except maybe that we have Param abbreviated in one of these (what about using the full name).

Another question is do we need any of these to be public? (Hmm, I see it used to be public so far).

@javornikolov
Copy link
Contributor

In general seems good to me. I guess we can merge it after polishing the naming.

@MMatten MMatten force-pushed the allow-variable-param-name-patterns branch from 3652629 to e8fb688 Compare February 7, 2018 22:04
@MMatten MMatten changed the title Draft for variable param names Allow parameter name patterns to be varied from and restored to default values. Feb 7, 2018
@MMatten
Copy link
Contributor Author

MMatten commented Feb 7, 2018

@javornikolov I still have to test this over some of the adapters but any review comments at this stage would be great.

@MMatten MMatten force-pushed the allow-variable-param-name-patterns branch from e8fb688 to ba98a1b Compare February 8, 2018 18:13
@MMatten
Copy link
Contributor Author

MMatten commented Feb 8, 2018

I'm not sure how my local build didn't pick up that unused import. Fixed and force pushed.

protected abstract Pattern getParameterPattern();
protected String defaultParamPatternString = "";

protected String getParamPatternString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep that method private (if we want a separate method at all)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see we're using it in subclasses. I'm wondering about the getParameterPattern().toString() alternative. My point is that having too many methods which is possible to override might lead to some confusion when implementing a new adapter.

defaultParamPatternString : Options.get(Options.OPTION_PARAMETER_PATTERN);
}

//protected abstract Pattern getParameterPattern();
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that comment redundant? Maybe we rather refer to defaultParamPatternString attribute as a comment here?

@@ -23,20 +22,15 @@

public DB2iEnvironment(String driverClassName) {
super(driverClassName);
defaultParamPatternString = "[@:]([A-Za-z0-9_]+)";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll delete this extra blank line.

return paramRegex;
protected String parseCommandText(String commandText) {
commandText = commandText.replaceAll(getParamPatternString(), "?");
return super.parseCommandText(commandText);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javornikolov I'm thinking that now I've got this block in every adapter I should merge it into the super-class's method.

@MMatten MMatten force-pushed the allow-variable-param-name-patterns branch 2 times, most recently from 1896078 to 13d973a Compare February 9, 2018 21:46
@MMatten
Copy link
Contributor Author

MMatten commented Feb 10, 2018

@javornikolov I've now tested the current PR state with all adapters.

@MMatten MMatten force-pushed the allow-variable-param-name-patterns branch from 13d973a to a786411 Compare February 12, 2018 18:56
@MMatten
Copy link
Contributor Author

MMatten commented Feb 12, 2018

@javornikolov I think I've tidied things up a little. What do you think about the current methods and visibility of them in AbstractDbEnvironment?

@MMatten
Copy link
Contributor Author

MMatten commented Feb 21, 2018

@javornikolov do you think this is close to being mergeable yet?

@javornikolov
Copy link
Contributor

@MMatten, sorry for the delay! Thank you for the reminder :-)
It looks OK to me though I have some wondering about about protected attributes vs methods: but it's theoretica for now. I'm merging this PR.

@javornikolov javornikolov added this to the 4.0.0 milestone Feb 21, 2018
@javornikolov javornikolov merged commit 59114d0 into master Feb 21, 2018
@javornikolov javornikolov deleted the allow-variable-param-name-patterns branch February 21, 2018 21:26
@Arun-Karunakaran
Copy link

Hi Team,

I am trying to explore how can i connect to command line terminal and execute unix/shell commands.
Please help suggest me with a sample example on how to do it. I am exploring on this for past several days. I tried even with downloading the latest jar file commandlinefixture.jar but it is throwing error while i am using it in dbfit syntax. Can you elaborate on the steps to perform it.
Thanks & regards,
Arun K

@MMatten
Copy link
Contributor Author

MMatten commented Mar 15, 2018 via email

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.

None yet

3 participants