Updated TypeProvider to support different method/function parameters #67

CarsonF · 2017-06-03T20:13:39Z

Updated TypeProvider to support different method/function parameters besides the first with "index" parameter.

Fixes #20

This is my first PR for an idea plugin, so advice is very much appreciated.

It seems like when creating the signature, we don't know the class name. So we need to add the types of all indexes that could be needed and match it later in getBySignature. I did that by concatenating each index and its type and joining those with the trimKey and then joining that to the refSignature with two of the trimKey chars. This breaks if the index wanted is greater than 9 but I figured that was an ok risk. I'm happy to change it to another kind of pattern, I'm just not sure what I can use to delineate on here.

I don't know what kind of BC we are trying to provide here. This does break BC for PhpTypeProviderUtil if it was called directly. Nothing has changed with PhpToolboxTypeProviderInterface which I figure is the one everyone is using.

Haehnchen

welcome to the club. ;)

a test would be nice. also you broken some function type tests

Haehnchen · 2017-06-04T10:24:07Z

src/de/espend/idea/php/toolbox/type/utils/PhpTypeProviderUtil.java


-        PsiElement parameter = parameters[0];
+        HashMap<Integer, String> parameterSignatures = new HashMap<>();


switch HashMap to Map for variable declaration

Haehnchen · 2017-06-04T10:29:34Z

src/de/espend/idea/php/toolbox/type/utils/PhpTypeProviderUtil.java

+                .map(entry -> entry.getKey() + entry.getValue())
+                .collect(Collectors.joining(String.valueOf(trimKey)));
+
+        return refSignature + trimKey + trimKey + parametersSignature;


why are you using a double trimKey?

So it can be split on in getBySignature. I know this is weird. I'm joining multiple parameter pairs with the trim key as well, so I couldn't split on that one character alone.

This doThing('Foo', 'Bar') looks like this {refSignature} ƕƕ 0Foo ƕ 1Bar (added spaces for clarity).

As I said in the PR description, I'm happy to do it another way. I'm just not sure which characters would be allowed that wouldn't mess up other things.

whats about this solution?

{refSignature}ƕ0#Fooƕ1#Bar

Find first special char and then split it. in additional we support +9 parameter also, as you extracting just the first char for parameter index

CarsonF · 2017-06-04T16:11:27Z

a test would be nice. also you broken some function type tests

I'm struggling to get the tests running locally.

ERROR: Problems found loading plugins:<p/>
Plugin "PHP" was not loaded: required plugin "com.intellij.java-i18n" not installed.<p/>
Plugin "PHP Toolbox" was not loaded: required plugin "com.intellij.java-i18n" not installed.<p/>
<br><a href="disable">Disable not loaded plugins</a><p/><a href="edit">Open plugin manager</a>

I've add java-i18n from IntelliJ but something else must be wrong. I can run/debug the plugin so I've got at least some of the other dependencies right.

Haehnchen · 2017-06-06T16:03:41Z

So my project configuration about testing

Haehnchen · 2017-06-06T16:13:07Z

src/de/espend/idea/php/toolbox/type/utils/PhpTypeProviderUtil.java

@@ -16,46 +17,66 @@

    @Nullable
    public static String getReferenceSignature(FunctionReference methodReference, char trimKey) {
-        return getReferenceSignature(methodReference, trimKey, 1);
+        return getReferenceSignature(methodReference, trimKey, 0);


i think current tests failing because of this change. as index starts by 0

CarsonF · 2017-06-10T16:01:36Z

Ah ok so adding those to the SDK fixed it...but I'm not sure that's correct. The SDK makes them like global dependencies, right? This leaves me wondering what the java-i18n, properties, junit, css, php dependencies are for if they are specified in the SDK.

Anyways I can work on the tests now, and I'll keep playing with the dependencies. Glad I don't have to be the one to make sure the compilation is correct ;)

CarsonF · 2017-06-10T17:04:25Z

Ok tests caught a bug I had, fixed that and added tests for new functionality. Tests pass locally, Travis is just being derpy.

Haehnchen · 2017-06-11T08:47:35Z

@CarsonF travis build environment fixed in master please rebase. in additional a simple example with the new index key inside README.md would be nice, then we are ready to merge and release this 👍

Haehnchen · 2017-06-11T08:51:23Z

i also dont know why dependencies must declared in this way. but its working ;)

…besides the first with "index" parameter.

CarsonF · 2017-06-11T15:44:04Z

Done! 😄

Haehnchen · 2017-06-11T17:09:51Z

thx! will build new release soon

Koc · 2017-06-11T19:15:30Z

Nice to see new contributor

CarsonF · 2017-06-16T15:49:06Z

@Haehnchen thanks for 2faefdb. I thought removeIf(Objects::isNull) would be enough, but clearly not :)

Haehnchen · 2017-06-16T16:18:41Z

problem was:

part -> Integer.parseInt(part.substring(0, 1)),
part -> PhpTypeProviderUtil.getResolvedParameter(phpIndex, part.substring(1))

empty string provided, had no idea why, but lesson learned in all plugins: try to validate everything ;)

Haehnchen reviewed Jun 4, 2017

View reviewed changes

Haehnchen reviewed Jun 6, 2017

View reviewed changes

CarsonF force-pushed the return-type-index branch 2 times, most recently from 4d50be6 to 24d22cb Compare June 10, 2017 16:59

CarsonF force-pushed the return-type-index branch from 24d22cb to dc76fe1 Compare June 11, 2017 15:42

Updated TypeProvider to support different method/function parameters …

68c468f

…besides the first with "index" parameter.

CarsonF force-pushed the return-type-index branch from dc76fe1 to 68c468f Compare June 11, 2017 15:43

Haehnchen merged commit 5f0500a into Haehnchen:master Jun 11, 2017

CarsonF deleted the return-type-index branch June 11, 2017 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated TypeProvider to support different method/function parameters #67

Updated TypeProvider to support different method/function parameters #67

CarsonF commented Jun 3, 2017

Haehnchen left a comment

Haehnchen Jun 4, 2017

CarsonF Jun 4, 2017

Haehnchen Jun 4, 2017

CarsonF Jun 4, 2017

Haehnchen Jun 6, 2017

CarsonF commented Jun 4, 2017

Haehnchen commented Jun 6, 2017

Haehnchen Jun 6, 2017

CarsonF commented Jun 10, 2017

CarsonF commented Jun 10, 2017

Haehnchen commented Jun 11, 2017

Haehnchen commented Jun 11, 2017

CarsonF commented Jun 11, 2017

Haehnchen commented Jun 11, 2017

Koc commented Jun 11, 2017

CarsonF commented Jun 16, 2017

Haehnchen commented Jun 16, 2017


		PsiElement parameter = parameters[0];
		HashMap<Integer, String> parameterSignatures = new HashMap<>();

Updated TypeProvider to support different method/function parameters #67

Updated TypeProvider to support different method/function parameters #67

Conversation

CarsonF commented Jun 3, 2017

Haehnchen left a comment

Choose a reason for hiding this comment

Haehnchen Jun 4, 2017

Choose a reason for hiding this comment

CarsonF Jun 4, 2017

Choose a reason for hiding this comment

Haehnchen Jun 4, 2017

Choose a reason for hiding this comment

CarsonF Jun 4, 2017

Choose a reason for hiding this comment

Haehnchen Jun 6, 2017

Choose a reason for hiding this comment

CarsonF commented Jun 4, 2017

Haehnchen commented Jun 6, 2017

Haehnchen Jun 6, 2017

Choose a reason for hiding this comment

CarsonF commented Jun 10, 2017

CarsonF commented Jun 10, 2017

Haehnchen commented Jun 11, 2017

Haehnchen commented Jun 11, 2017

CarsonF commented Jun 11, 2017

Haehnchen commented Jun 11, 2017

Koc commented Jun 11, 2017

CarsonF commented Jun 16, 2017

Haehnchen commented Jun 16, 2017