PiranhaJava // config: add returnType, receiverType and argumentIndex; move to properties.json #39

pranavsb · 2020-05-24T17:41:36Z

This is an enhancement and a fix for #38 and #6 .
The config file is moved to properties.json, removing the necessity for piranha.properties.

Return type and receiver type regexes can be specified in properties.json corresponding to the flagType and methodName. Argument index to check against xpFlagName can also be specified. All 3 are optional fields.

Please see TODO in XPFlagCleanerTest. I have changed getXPAPI to return API.UNKNOWN if the specified method's receiver or return type don't match what is specified in properties.json, but I think there is some change required in some other part of the code.
UPDATE: Have raised #40 for any further discussion related to this.

~~NOTE: Argument index is not yet implemented, so #6 is still open.~~

CLAassistant · 2020-05-24T17:41:41Z

All committers have signed the CLA.

mkr-plse

Thanks for an excellent enhancement PR. Overall, looks great.
Can you clarify some of the questions raised in the review?

java/README.md

java/piranha/config/properties.json

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java

java/piranha/src/test/resources/config/properties_test_receive.json

java/piranha/build.gradle

pranavsb · 2020-05-26T06:45:34Z

Thanks for an excellent enhancement PR. Overall, looks great.
Can you clarify some of the questions raised in the review?

Thank you. I've replied to your comments and added one of my own.

Will change:

all maps to one map - for holding config data
null to ...

java/sample/config/properties.json

lazaroclapp

This is an amazing contribution overall. I did a first pass of commenting. Basically, I'd like to have some more structure to our fields than just one gigantic map encoding the full JSON, and also have some decoupling between config format and in-memory representation (so we can keep supporting .properties for existing users), other than that, it's mostly nits. @mkr-plse , what do you think?

java/piranha/build.gradle

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java

pranavsb · 2020-05-27T13:47:17Z

@mkr-plse @lazaroclapp

All good points, will get back to this over the weekend and wrap it up.

Wanted to bring one thing to your attention though.
I think some gradle changes are required to ensure that json jar is contained within the piranha jar generated by gradle jar. I tried copying the 0.0.4-SNAPSHOT jar to my local maven repository and compiled a dummy springboot project and got this error. Used to compile and have in place piranha updates before.

Error: An unhandled exception was thrown by the Error Prone static analysis plugin.
[ERROR] Please report this at https://github.com/google/error-prone/issues/new and include the following:
[ERROR]
[ERROR] error-prone version: 2.3.4
[ERROR] BugPattern: Piranha
[ERROR] Stack Trace:
[ERROR] java.lang.NoClassDefFoundError: org/json/simple/parser/JSONParser
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]

lazaroclapp · 2020-05-27T17:29:50Z

@mkr-plse @lazaroclapp

All good points, will get back to this over the weekend and wrap it up.

Wanted to bring one thing to your attention though.
I think some gradle changes are required to ensure that json jar is contained within the piranha jar generated by gradle jar. I tried copying the 0.0.4-SNAPSHOT jar to my local maven repository and compiled a dummy springboot project and got this error. Used to compile and have in place piranha updates before.
Error: An unhandled exception was thrown by the Error Prone static analysis plugin.
[ERROR] Please report this at https://github.com/google/error-prone/issues/new and include the following:
[ERROR]
[ERROR] error-prone version: 2.3.4
[ERROR] BugPattern: Piranha
[ERROR] Stack Trace:
[ERROR] java.lang.NoClassDefFoundError: org/json/simple/parser/JSONParser
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]

How are you bringing in Piranha? Are you making it a dependency for your project? (If so, I believe the build system would grab its dependencies from maven central) Or are you just sticking the jar somewhere? (Piranha is not supposed to be a fat jar, afaik)

Note that the sample projects in this repo build by simply depending on Piranha and we'd be sure to test this with our internal (buck) build system before cutting a release.

What happens if you replace -SNAPSHOT with .1-LOCAL, do ./gradlew install (on the java/ directory) then import in your project from the local maven cache?

pranavsb · 2020-05-28T08:10:22Z

How are you bringing in Piranha? Are you making it a dependency for your project? (If so, I believe the build system would grab its dependencies from maven central) Or are you just sticking the jar somewhere? (Piranha is not supposed to be a fat jar, afaik)

Yes, Piranha is added as a dependency. Basically the same POM config that's given in the Java README. I figured out that adding json-simple to annotationProcessorPaths (that already contains piranha and errorprone) works.

Note that the sample projects in this repo build by simply depending on Piranha and we'd be sure to test this with our internal (buck) build system before cutting a release.

👍

What happens if you replace -SNAPSHOT with .1-LOCAL, do ./gradlew install (on the java/ directory) then import in your project from the local maven cache?

This works too! Must have been some issue on my end.

lazaroclapp · 2020-05-28T17:28:15Z

How are you bringing in Piranha? Are you making it a dependency for your project? (If so, I believe the build system would grab its dependencies from maven central) Or are you just sticking the jar somewhere? (Piranha is not supposed to be a fat jar, afaik)

Yes, Piranha is added as a dependency. Basically the same POM config that's given in the Java README. I figured out that adding json-simple to annotationProcessorPaths (that already contains piranha and errorprone) works.

Note that the sample projects in this repo build by simply depending on Piranha and we'd be sure to test this with our internal (buck) build system before cutting a release.

👍

What happens if you replace -SNAPSHOT with .1-LOCAL, do ./gradlew install (on the java/ directory) then import in your project from the local maven cache?

This works too! Must have been some issue on my end.

I think what is happening is, because you are moving the SNAPSHOT jar around, it was trying to use Piranha after this patch but only fetching the dependencies of the latest Piranha release, which doesn't include json-simple (you shouldn't need to add it to annotationProcessorPaths once this PR becomes part of mainline Piranha, and I should test that that is true before cutting a release :) )

pranavsb · 2020-06-02T15:45:42Z

Have updated the PR with support for argumentIndex.

This closes #6

Anything else remaining?
Not sure about the testcases with //BUG:
They pass for me, with and without the line, though I see from the code that maybe that's not supposed to happen.

Update: the README should be updated to mention the optional argumentIndex. I will do that.

lazaroclapp · 2020-06-02T19:21:11Z

Anything else remaining?
Not sure about the testcases with //BUG:
They pass for me, with and without the line, though I see from the code that maybe that's not supposed to happen.

Haven't done the full review yet, but two things:

Let's remove the //BUG: lines on the new refactoring test cases (no need to change the existing ones), if they aren't being checked. As long as we are checking input and output code, it should be good. Does that make sense to you as well, @mkr-plse ?
Please git pull to rebase atop master and resolve the conflicts with other recent PRs.

mkr-plse

Thanks @pranavsb for the PR again. The changes are looking good. Left a few comments and will review the tests after the comments are addressed.

java/README.md

java/piranha/config/properties.json

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

mkr-plse · 2020-06-02T20:13:53Z

Let's remove the //BUG: lines on the new refactoring test cases (no need to change the existing ones), if they aren't being checked. As long as we are checking input and output code, it should be good. Does that make sense to you as well, @mkr-plse ?

Agree.

Moved from defaultCharset to StandardCharsets.UTF_8 for reading properties.json

pranavsb · 2020-06-04T10:39:02Z

Let's remove the //BUG: lines on the new refactoring test cases
Please git pull to rebase atop master and resolve the conflicts with other recent PRs.

Done!

I also closed almost all review comments. Have left a few open in case there's further discussion required.

One thing worth noting, I've changed the Files.newBufferedReader that reads properties.json to use StandardCharsets.UTF_8 instead of the Charset.defaultCharset() call earlier.

mkr-plse

Thanks for the changes. Left a few more comments, but at the high level, these are the main issues:
a) We can get rid of the pattern for receiverType and returnType and simplify the code
b) We can change the return type of checkValue... to the appropriate Object. Will avoid redundant get calls.
c) The tests may need to be enhanced to check for a few more combinations. Some cases that I can think of:

when the required properties are not provided in the json,
when receiverType alone is specified
when returnType alone is specified
when argumentIndex alone is specified
when argumentIndex and receiverType are specified
when argumentIndex and returnType are specified
when all the three are specified
for each of the above cases, there are likely two tests -- with or without method matching in the input source

java/piranha/config/properties.json

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

java/piranha/src/test/resources/config/properties_test_argument.json

java/README.md

pranavsb · 2020-06-05T07:14:50Z

Thanks for the changes. Left a few more comments, but at the high level, these are the main issues:
a) We can get rid of the pattern for receiverType and returnType and simplify the code

Done.

b) We can change the return type of checkValue... to the appropriate Object. Will avoid redundant get calls.

Done.

c) The tests may need to be enhanced to check for a few more combinations. Some cases that I can think of:

for each of the ~~above~~ below cases, there are likely two tests -- with or without method matching in the input source

Testcases

when the required properties are not provided in the json,
when receiverType alone is specified
when returnType alone is specified
when argumentIndex alone is specified
For above 3, there's already properties_test_return, properties_test_receive and properties_test_argument. These files have methodName and flagType and one of the optional fields. But they test the no flag case. Will add similar checks with flag specified.
when argumentIndex and receiverType are specified
when argumentIndex and returnType are specified
when all the three are specified

Also, I've changed the init method in XPFlagCleaner from private to default (package-private), for better testing, hope that's okay.

mkr-plse · 2020-06-05T12:11:36Z

Thanks for the changes. Left a few more comments, but at the high level, these are the main issues:
a) We can get rid of the pattern for receiverType and returnType and simplify the code

Done.

@pranavsb Can you push your changes? It looks like the files have not been updated yet.

pranavsb · 2020-06-05T13:01:55Z

Thanks for the changes. Left a few more comments, but at the high level, these are the main issues:
a) We can get rid of the pattern for receiverType and returnType and simplify the code

Done.

@pranavsb Can you push your changes? It looks like the files have not been updated yet.

Was planning to finish tests before pushing. Have added a few.
~~Let me push for now.~~ Pushed.

mkr-plse · 2020-06-05T13:34:07Z

Was planning to finish tests before pushing. Have added a few.
~~Let me push for now.~~ Pushed.

I thought you forgot to push because of the done comment. No hurry. Let me know when the code is available for review again.

pranavsb · 2020-06-06T11:11:51Z

Added (lots of) tests and pushed. Guess it's in decent shape now.

mkr-plse

Changes look great. The elaborate tests and the comments accompanying them are very helpful. Thanks for the painstaking effort.

There are a couple of comments. It will be great if you can address them. We can merge the changes after @lazaroclapp also takes a look at the changes.

mkr-plse · 2020-06-08T12:39:07Z

java/README.md


-The `emptyMethods` specify the APIs which need to be discarded from the code. For example, a statement `enableFlag(SAMPLE_STALE_FLAG);` will be deleted from the code. 
+For `returnType` and `receiverType`, types should be written as `boolean` or `void` for inbuilt types, and fully qualified for custom defined types. eg: `com.uber.piranha.XPFlagCleanerPositiveCases.XPTest`


inbuilt -> primitive.

Also, question as a user reading this: what about String? Is it string or java.lang.String? (I'd assume the second, specially if we say "primitive" but might be worth calling it out explicitly.

Also added java.lang.string and a java.lang.boolean (which returns Boolean instead of boolean) test

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java

lazaroclapp

Overall, really happy with this version. There is a comment under noFlagCase that I probably want some more clarity on before landing.

The rest, I think, are mostly nits and minor fixes.

java/README.md

lazaroclapp · 2020-06-08T18:06:19Z

java/README.md


-The `emptyMethods` specify the APIs which need to be discarded from the code. For example, a statement `enableFlag(SAMPLE_STALE_FLAG);` will be deleted from the code. 
+For `returnType` and `receiverType`, types should be written as `boolean` or `void` for inbuilt types, and fully qualified for custom defined types. eg: `com.uber.piranha.XPFlagCleanerPositiveCases.XPTest`


Also, question as a user reading this: what about String? Is it string or java.lang.String? (I'd assume the second, specially if we say "primitive" but might be worth calling it out explicitly.

java/piranha/config/properties.json

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

java/piranha/src/test/resources/config/properties_test_invalid.json

lazaroclapp · 2020-06-08T19:27:07Z

java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java

+   * Methods with no arguments are simplified, provided argumentIndex is not specified
+   * (assuming that returnType and receiverType - if specified - are a match)
+   * Uses "properties_test_noFlag.json" as the config file.
+   * Note that "Piranha:FlagName" is not considered when argumentIndex is not specified.


Out of curiosity: will this work only for versions of the method that take zero args?

My worry here is that a misconfiguration, where Piranha:FlagName is empty, and the argumentIndex is missing, it will go and delete half the code. We probably don't want that behavior. @Murali ??

I think we'd want any code that would delete experimentation.isToggleEnabled(SOME_FLAG) { ... } for every possible value of SOME_FLAG at once to error out by default. If someone really needs to do this, we can add a Piranha:AllowMassRemoval=true argument or something. It just feels like the kind of thing that might scare new users from trying piranha if they don't configure it right.

Unless I am mistaken and this case will only delete treatment methods with zero args.

My understanding is that this will only delete methods listed in the properties.json without checking for flagname in argument. At worst, this will delete all calls to an API if the argumentIndex is not specified. Your point on getting the configuration right is valid too.

Let us leave this functionality as it is in the current version of the PR and if it becomes a source of confusion, let us revisit this.

I feel that worst case scenario is really really bad. Consider the effects of trying out Piranha in Uber's monorepos under those conditions, it would locally rewrite maybe hundreds of thousands of lines of code... and scare many people from ever trying it again.

I am fine with that check being a follow-up task, but I'd want to have it in place before we cut any release with this (it's already tricky enough that we are switching the config file format)

Also for a follow up task (I can do it): Print a message whenever the config points to a file ending in .properties explaining the format change and linking the docs in case someone is just upgrading Piranha.

How about this:
a) By default, argumentIndex is a required parameter in json.
b) Using a configuration option (as you point above), this can be made optional.

(a) will be good for most users and in cases, where a user wants to delete all calls to the API, (b) can be used.

Yup. That's exactly what I'd want. And we can add (a) to this PR and make (b) a follow up, to make this easier to land. But I'd rather have that small guardrail there than the extra functionality of deleting methods without arguments 😄

^ @pranavsb Everything else is a nit and could be done later, but I'd like this one change before landing:

a) By default, argumentIndex is a required parameter in json.

Also for a follow up task (I can do it): Print a message whenever the config points to a file ending in .properties explaining the format change and linking the docs in case someone is just upgrading Piranha.

Raised #49 for this. Clearly, I have issues :P

lazaroclapp · 2020-06-08T19:28:08Z

java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java

+              "class XPFlagCleanerSinglePositiveCase {",
+              " private XPTest experimentation;",
+              " public String evaluate() {",
+              "  if (experimentation.isToggleEnabled()) { int a = 1; }",


What does this code do about experimentation.isToggleEnabled(SOME_FLAG) { ... }?

As you know by now, argumentIndex not being specified would lead to all occurrences of isToggleEnabled being removed, irrespective of its argument(s) count/value.

I'm in the process of adding ArgumentIndexOptional Piranha argument to fix this

java/piranha/src/test/java/com/uber/piranha/XPFlagCleanerTest.java

lazaroclapp

Mostly just the not allowing index to be missing change (see discussion above). You don't need to add the option to re-enable the empty index mode, and you can decide whether or not to address the other nits, but this one is kinda important :/

pranavsb · 2020-06-09T08:46:06Z

Mostly just the not allowing index to be missing change (see discussion above). You don't need to add the option to re-enable the empty index mode, and you can decide whether or not to address the other nits, but this one is kinda important :/

Alright I see your point about the mass deletion being confusing for new users.
But if I make argumentIndex mandatory and not implement "enable empty index", Piranha would no longer work for my codebase :/

How about I make argumentIndex mandatory, but it allows values from -1,0,1...?
-1 would correspond to the noFlagCase, that is, a toggle method that takes no arguments.
Later, the Piranha options can be modified and argumentIndex can go back to being 0 upwards if needed.

P.S. I'll need a few days before starting on this, but I'll be happy to work on the nits too. Let me know if you catch anything else. :)

mkr-plse · 2020-06-09T12:36:10Z

Mostly just the not allowing index to be missing change (see discussion above). You don't need to add the option to re-enable the empty index mode, and you can decide whether or not to address the other nits, but this one is kinda important :/

Alright I see your point about the mass deletion being confusing for new users.
But if I make argumentIndex mandatory and not implement "enable empty index", Piranha would no longer work for my codebase :/

How about I make argumentIndex mandatory, but it allows values from -1,0,1...?
-1 would correspond to the noFlagCase, that is, a toggle method that takes no arguments.

Instead of -1, see the suggestion here which references this. We can add an option ArgumentIndexOptional to Piranha and it will be false by default. This will avoid the confusion related to misconfiguration. For your codebase (and similar scenarios), the option can be set to true when Piranha is invoked.

By adding this option, you can also leave most of the noFlagTestCases as is and simply update the configuration options passed to the tests.

P.S. I'll need a few days before starting on this, but I'll be happy to work on the nits too. Let me know if you catch anything else. :)

pranavsb · 2020-06-09T12:42:25Z

Instead of -1, see the suggestion here which references this. We can add an option ArgumentIndexOptional to Piranha and it will be false by default. This will avoid the confusion related to misconfiguration. For your codebase (and similar scenarios), the option can be set to true when Piranha is invoked.

By adding this option, you can also leave most of the noFlagTestCases as is and simply update the configuration options passed to the tests.

Yes, that's what I meant by "enable empty index". Ok then, new Piranha option is the way. ArgumentIndexOptional is a good name. Will send updated PR.

lazaroclapp · 2020-06-09T17:40:22Z

Instead of -1, see the suggestion here which references this. We can add an option ArgumentIndexOptional to Piranha and it will be false by default. This will avoid the confusion related to misconfiguration. For your codebase (and similar scenarios), the option can be set to true when Piranha is invoked.
By adding this option, you can also leave most of the noFlagTestCases as is and simply update the configuration options passed to the tests.

Yes, that's what I meant by "enable empty index". Ok then, new Piranha option is the way. ArgumentIndexOptional is a good name. Will send updated PR.

Sorry for that extra work, but it's a better long term solution and, compared with the rest of this PR, just adding that option will be a fairly small change, I think. I would not be comfortable releasing a new version of Piranha with "empty index" allowed by default, even if we could land this to master.

pranavsb · 2020-06-14T14:05:54Z

Pushed support for ArgumentIndexOptional and - brace yourself - more tests!

~~Update: the build is failing. Probably a config / goJF issue. Looking into it.~~

lazaroclapp

This is basically ready to land. I have just two short nit fixes and a question.

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

lazaroclapp · 2020-06-15T16:13:16Z

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

        }
+        methodPropertiesForMethodName.add(methodProperty);
+      } else {


For readability, consider reversing this conditional, so you have:

if (methodName == null || getValueStringFromMap(methodProperty, FLAG_TYPE_KEY) == null) { throw ...; } if (!isArgumentIndexOptional && getArgumentIndexFromMap(methodProperty) == null) { throw ...; } List<Map<String, Object>> methodPropertiesForMethodName = ...; ...

Done.
Also replaced the add methodProperty/new ArrayList logic with a one-liner using computeIfAbsent

lazaroclapp · 2020-06-15T16:14:37Z

java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java

@@ -372,15 +386,17 @@ private API getXPAPI(
      // if it's not a match, skip to next method property map
      String returnType = getValueStringFromMap(currentMethodProperties, RETURN_TYPE_STRING);
      if (returnType != null) {
-        if (!isMethodTreeMatchesMethodProperty(mst, RETURN_TYPE_STRING, returnType)) {
+        String mReturn = ASTHelpers.getReturnType(mst).toString();
+        if (!returnType.equalsIgnoreCase(mReturn)) {


Why equalsIgnoreCase instead of case sensitive equals?

Just to allow users the flexibility of wrongly capitalizing stuff. Like java.lang.string or the fully qualified name of their class.

@lazaroclapp - waiting for your followup - if any - before pushing.

I say let's compare case-sensitive, since Java itself is case sensitive, I think, e.g. you can have both a type com.foo.Bar and one called com.foo.bar at the same time in the same JVM (you probably shouldn't, but still). That said, I have no super strong opinion here. cc: @mkr-plse Any thoughts?

...you can have both a type com.foo.Bar and one called com.foo.bar ...

Oh, didn't know this. Then a case sensitive match is a better default for now - just in case someone does this.

Updated and fixed testcase.

pranavsb · 2020-06-15T20:48:37Z

Pushed case-sensitive match for returnType and receiverType (and some refactoring)

lazaroclapp

LGTM!! 😄 🎉

lazaroclapp · 2020-06-15T20:56:22Z

Congrats and thank you for the massive contribution @pranavsb !

pranavsb · 2020-06-15T21:03:21Z

Congrats and thank you for the massive contribution @pranavsb !

Thanks for all your help. It was good fun! :)
@lazaroclapp @mkr-plse

mkr-plse · 2020-06-15T21:05:54Z

Indeed, this is a massive contribution. Thanks a lot @pranavsb. Looking forward to more such contributions ;)

pranavsb changed the title ~~config: add returnType and receiverType; move to properties.json~~ PiranhaJava // config: add returnType and receiverType; move to properties.json May 24, 2020

mkr-plse suggested changes May 25, 2020

View reviewed changes

pranavsb commented May 26, 2020

View reviewed changes

java/piranha/build.gradle Show resolved Hide resolved

mkr-plse reviewed May 26, 2020

View reviewed changes

java/sample/config/properties.json Show resolved Hide resolved

pranavsb mentioned this pull request May 26, 2020

PiranhaJava // Two pass analysis for accurate variable deletion #40

Open

lazaroclapp reviewed May 26, 2020

View reviewed changes

pranavsb mentioned this pull request May 30, 2020

PiranhaJava // Use Immutable structures (List/Map) for config data #41

Closed

mkr-plse suggested changes Jun 2, 2020

View reviewed changes

pranavsb added 6 commits June 3, 2020 10:58

add no flag case in getXPAPI

f9847f5

Add receiver and return type support; move config to properties.json

7ba5256

Add properties.json to sample; delete old config

62b200c

Add properties.json to sample; delete old config

e13b58a

Support argumentIndex in properties.json

230a938

Update argumentIndex in README

ef06c2b

pranavsb changed the title ~~PiranhaJava // config: add returnType and receiverType; move to properties.json~~ PiranhaJava // config: add returnType, receiverType and argumentIndex; move to properties.json Jun 3, 2020

Rebase master, review comments and refactoring

c89c433

Moved from defaultCharset to StandardCharsets.UTF_8 for reading properties.json

pranavsb force-pushed the pranav_add_properties_json_2 branch from 47ffe00 to c89c433 Compare June 4, 2020 10:30

mkr-plse suggested changes Jun 4, 2020

View reviewed changes

Increase test coverage, refactoring

36dbefd

Increase test coverage, refactoring

76c0c8c

mkr-plse reviewed Jun 8, 2020

View reviewed changes

mkr-plse approved these changes Jun 8, 2020

View reviewed changes

lazaroclapp reviewed Jun 8, 2020

View reviewed changes

lazaroclapp suggested changes Jun 8, 2020

View reviewed changes

This was referenced Jun 13, 2020

PiranhaJava // Decouple configMethodProperties from its JSON representation #48

Closed

PiranhaJava // print warning when .properties is used after v0.0.4 #49

Closed

Add ArgumentIndexOptional; add tests

0af7af9

pranavsb force-pushed the pranav_add_properties_json_2 branch from c35c674 to 0af7af9 Compare June 14, 2020 14:16

mkr-plse mentioned this pull request Jun 15, 2020

PiranhaObjC : Unclear usage documentation #50

Closed

lazaroclapp reviewed Jun 15, 2020

View reviewed changes

Case-sensitive match return and receiver; refactor

ff59d06

lazaroclapp approved these changes Jun 15, 2020

View reviewed changes

lazaroclapp merged commit e6c088d into uber:master Jun 15, 2020

This was referenced Jun 15, 2020

Support specifying method and argument index pairs in piranha.properties #6

Closed

PiranhaJava - remove flag methods that have no arguments (that don't use enum) #38

Closed

mkr-plse mentioned this pull request Jun 15, 2020

Adding support for control methods with more than 1 argument #36

Closed


		The `emptyMethods` specify the APIs which need to be discarded from the code. For example, a statement `enableFlag(SAMPLE_STALE_FLAG);` will be deleted from the code.
		For `returnType` and `receiverType`, types should be written as `boolean` or `void` for inbuilt types, and fully qualified for custom defined types. eg: `com.uber.piranha.XPFlagCleanerPositiveCases.XPTest`

PiranhaJava // config: add returnType, receiverType and argumentIndex; move to properties.json #39

PiranhaJava // config: add returnType, receiverType and argumentIndex; move to properties.json #39

Conversation

pranavsb commented May 24, 2020 • edited

CLAassistant commented May 24, 2020 • edited

mkr-plse left a comment

Choose a reason for hiding this comment

pranavsb commented May 26, 2020 • edited

lazaroclapp left a comment

Choose a reason for hiding this comment

pranavsb commented May 27, 2020

lazaroclapp commented May 27, 2020

pranavsb commented May 28, 2020

lazaroclapp commented May 28, 2020

pranavsb commented Jun 2, 2020 • edited

lazaroclapp commented Jun 2, 2020

mkr-plse left a comment

Choose a reason for hiding this comment

mkr-plse commented Jun 2, 2020

pranavsb commented Jun 4, 2020 • edited

mkr-plse left a comment

Choose a reason for hiding this comment

pranavsb commented Jun 5, 2020 • edited

Testcases

mkr-plse commented Jun 5, 2020

pranavsb commented Jun 5, 2020 • edited

mkr-plse commented Jun 5, 2020

pranavsb commented Jun 6, 2020

mkr-plse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavsb Jun 13, 2020 • edited

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

pranavsb commented Jun 9, 2020 • edited

mkr-plse commented Jun 9, 2020

pranavsb commented Jun 9, 2020

lazaroclapp commented Jun 9, 2020

pranavsb commented Jun 14, 2020 • edited

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavsb Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavsb commented Jun 15, 2020

lazaroclapp left a comment

Choose a reason for hiding this comment

lazaroclapp commented Jun 15, 2020

pranavsb commented Jun 15, 2020

mkr-plse commented Jun 15, 2020

pranavsb commented May 24, 2020 •

edited

CLAassistant commented May 24, 2020 •

edited

pranavsb commented May 26, 2020 •

edited

pranavsb commented Jun 2, 2020 •

edited

pranavsb commented Jun 4, 2020 •

edited

pranavsb commented Jun 5, 2020 •

edited

pranavsb commented Jun 5, 2020 •

edited

pranavsb Jun 13, 2020 •

edited

pranavsb commented Jun 9, 2020 •

edited

pranavsb commented Jun 14, 2020 •

edited

pranavsb Jun 15, 2020 •

edited