Closed xristy closed 7 years ago
@rnewson would you have time to review this PR?
Later this week hopefully, can't right now. Sounds awesome though!
Sent from my iPhone
On 17 Apr 2017, at 13:27, Elie Roux notifications@github.com wrote:
@rnewson would you have time to review this PR?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Ok thanks a lot!
One quick comment. There's a lot of white space changes that make this look bigger than it is. Can you please update the relevant commits to eliminate this please? Notably in Analyzers.java itself.
You also upgrade the json library version. A separate commit in the pr for that please, including reason.
More later when I can focus, and thanks again for getting this started.
Sent from my iPhone
On 17 Apr 2017, at 13:48, Elie Roux notifications@github.com wrote:
Ok thanks a lot!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
I've cleaned up Analyzers.java to reflect just the functional changes.
I've reverted the org.json to 20090211. I had updated to the most recent simply to have access the source and docs since I was not familiar with the lib and there is no source associated with 20090211.
Thank you for your very thorough consideration of the pull request. I've created a new branch in my fork and made 3 clean commits: 1) implementing the analyzer configuration; 2) adding tests; and 3) adding documentation. I tried to answer all of the comments in the new branch. I was thinking to close this PR and make a new one with the new "clean" branch. I ended up making far too many changes to be comfortable trying to do everything with fixup and so on. Let me know if this approach will work for you.
hi, sorry for the delay in responding, I only get the weekends to hack on this project. Should I be reviewing PR 249 instead of this one?
Yes please review PR # 249. I thought it would be more effective for me to create a new branch and PR than to try and repair commit history and such, especially since made more changes while tidying up the code.
I've replied to several of your comments on this PR in the various comments but the actual changes I made are reflected in PR # 249.
I appreciate very much the time you're taking to review this work. I think the extension should be useful to others.
Thanks. Will review new pr later today.
Sent from my iPhone
On 23 Apr 2017, at 16:20, Chris Tomlinson notifications@github.com wrote:
Yes please review PR # 249. I thought it would be more effective for me to create a new branch and PR than to try and repair commit history and such, especially since made more changes while tidying up the code.
I've replied to several of your comments on this PR in the various comments but the actual changes I made are reflected in PR # 249.
I appreciate very much the time you're taking to review this work. I think the extension should be useful to others.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
This PR is superseded by PR 249
This PR implements configuration driven use of analyzers as suggested in Issue 245.
AnalyzerTests.java is updated with appropriate tests and CONFIGURING_ANALYZERS.md documents the extension.
The org.json artifact was updated to 20160810.
The version was updated to 2.2.0-SNAPSHOT.