Modify to identify only the changed lines from a given patch #2

kasunsiyambalapitiya · 2017-04-18T13:20:01Z

Previous program considered a line range containing the modified lines from the given patch, for finding the authors, author commits and approved reviewers. As asked in the second code review the code is changed to detect exact lines been modified from the given patch and find authors, author commits and approved reviewers of those lines. This PR also contains all the modifications asked in PR1 https://github.com/wso2-incubator/code-quality-matrices/pull/1

Note:
At present the program works well for all WSO2 patches except the patches containing octopus merge commits ( commits having more than 2 parents) which may rarely available in WSO2 repositories

Add log messages for all classes

in the pull request kasunsiyambalapitiya/CodeQualityMatricesProject#1 format the code

added streams for saveReviewersToList method added streams for savePrNumberAndRepoName method replaced for loop with for each loop in readTheReviewOutJSON method

add constants for Reviewers.java

Handle the response code recieved from Graphql API Correct the message thrown in Exception

Shan1024 · 2017-04-18T16:03:21Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ReviewAnalyser.java

+                        try {
+                            String jsonText = githubApiCaller.callReviewApi(repositoryName, prNumber, githubToken);
+                            if (jsonText != null) {
+//                                Type listType = new TypeToken<List<ReviewApiResponse>>() {


Remove commented codes.

Fixed in 698c71e

Shan1024 · 2017-04-18T16:04:01Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/model/Author.java

+public class Author {
+
+    @SerializedName("date")
+


Unnecessary newline.

Fixed in 698c71e

Shan1024 · 2017-04-18T16:04:26Z

...litymatrices/src/main/java/com/wso2/code/quality/metrics/model/CommitHistoryApiResponse.java

+    private String commitHash;
+
+    @SerializedName("commit")
+


Unnecessary newline.

Fixed in 698c71e

Shan1024 · 2017-04-18T16:06:52Z

...so2.codequalitymatrices/src/test/java/com/wso2/code/quality/metrics/GithubResponsesTest.java

+    @Test
+    public void testSaveRepoNames() throws CodeQualityMetricsException {
+        Map<String, List<String>> commitHashWithRepoNames = new HashMap<>();
+        commitHashWithRepoNames.put("eaa45529cbabc5f30a2ffaa4781821ad0a5223ab",


These hashes seems to be used in multiple places. So might be able to use a constant.

chamilad · 2017-04-18T16:10:33Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ApiUtility.java

+/**
+ * This is a utility class for calling APIs.
+ *
+ * @since 1.0.0


This doc line is not used AFAIR. Better to check for consistency with other repositories.

I checked with few WSO2 repos, some contain @SInCE tag some doesn't, since this is a class level doc comment I think it is better to have it, What is your opinion?

@kasunsiyambalapitiya : I think what chamila meant was don't use "This" word in the doc section. We all know that the doc is related to "This". So we can just say "Utility class for calling APIs".

chamilad · 2017-04-18T16:12:52Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ApiUtility.java

+        BufferedReader bufferedReader = null;
+        CloseableHttpClient httpClient;
+        CloseableHttpResponse httpResponse = null;
+        httpClient = HttpClients.createDefault();


Any reason not to instantiate at the same line where this is declared?

Fixed in 698c71e

chamilad · 2017-04-18T16:13:56Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ApiUtility.java

+        String jsonText;
+        try {
+            httpResponse = httpClient.execute(httpGet);
+            int responseCode = httpResponse.getStatusLine().getStatusCode();


Is it possible for httpResponse.getStatusLine() to return null? If so this might be a possible NPE producer.

Handled NPE in 698c71e

chamilad · 2017-04-18T16:15:19Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ApiUtility.java

+            int responseCode = httpResponse.getStatusLine().getStatusCode();
+            if (responseCode == 200) {
+                //success
+                bufferedReader = new BufferedReader(new InputStreamReader(httpResponse.getEntity().getContent(),


Same with httpResponse.getEntity(). When chaining calls, better to verify for possible NPEs.

Fixed in 698c71e

chamilad · 2017-04-18T16:20:40Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/Application.java

+        if (args.length == 3) {
+            String pmtToken = args[0];
+            String patchId = args[1];
+            String gitHubToken = args[2];


IMHO, it might be better to offload tokens to a config file and only accept the patch ID as an argument, as tokens tend to be reused again and would be cumbersome to enter in the command line.

Fixed in 698c71e

chamilad · 2017-04-18T16:24:20Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/GithubApiCaller.java

+ *
+ * @since 1.0.0
+ */
+public class GithubApiCaller {


This looks like a util class and could be final and methods could be static as there doesn't seem to be any state to be preserved between calls.

Converted to a Util class in 698c71e

chamilad · 2017-04-18T16:26:19Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/PmtApiCaller.java

+ * @since 1.0.0
+ */
+public class PmtApiCaller {
+    public PmtApiCaller() {


Why is this constructor declared? Leftover from any refactoring?

:) Removed in 698c71e

chamilad · 2017-04-18T16:29:18Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ReviewAnalyser.java

+     * @return a map of pull requests againt their repository name
+     * @throws CodeQualityMetricsException results
+     */
+    Map<String, Set<Integer>> savePrNumberAndRepoName(String jsonText) throws CodeQualityMetricsException {


Should this be default access? Can it be private?

Since this method is called from a test case private access cannot be given

@kasunsiyambalapitiya : You can give private access even it is used in a test case. AFAIK, you can use java reflection to temporary change the access modifier before calling this method from the test case.

chamilad · 2017-04-18T16:32:45Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/model/Constants.java

+ * under the License.
+ */
+
+package com.wso2.code.quality.metrics.model;


If this class contains all the constants used in the project, then it might belong at the parent level of the package hierarchy, or under a separate util package.

chamilad · 2017-04-18T16:33:19Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/model/Graphql.java

+    private String graphqlInputWithoutHistory;
+    private static final long serialVersionUID = 42L;
+
+    public Graphql() {


Unnecessary declaration?

Without it maven build fails giving this error com.wso2.code.quality.metrics.model.Graphql is Serializable; consider declaring a serialVersionUID [com.wso2.code.quality.metrics.model.Graphql] At Graphql.java:[lines 32-44] SE_NO_SERIALVERSIONID. I fixed it as mentioned in the answer by "Jon Skeet" in http://stackoverflow.com/questions/285793/what-is-a-serialversionuid-and-why-should-i-use-it or else we need to avoid implementing Serializable interface. What is the best way to handle it?

chamilad · 2017-04-18T16:34:28Z

...so2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/model/GraphqlCommit.java

+public class GraphqlCommit {
+    @SerializedName("author")
+    private GraphqlAuthor author;
+    @SerializedName("history")


Better to keep a new line between properties to improve readability.

Fixed in 698c71e

chamilad · 2017-04-18T16:36:16Z

org.wso2.codequalitymatrices/src/main/resources/log4j.properties

+log4j.appender.CONSOLE= org.apache.log4j.ConsoleAppender
+log4j.appender.CONSOLE.Target=System.out
+log4j.appender.CONSOLE.layout=org.apache.log4j.PatternLayout
+log4j.appender.CONSOLE.layout.conversionPattern=%d{yyyy-MM-dd}-5p--10c:%m%n





Unnecessary new lines?

Fixed in 698c71e

chamilad · 2017-04-18T16:39:17Z

org.wso2.codequalitymatrices/src/test/java/com/wso2/code/quality/metrics/Token.java

+ * @since 1.0.0
+ */
+public class Token {
+    final String pmtToken = "tQU5vxzrGeBpLMQuwOsJW_fyYLYa";


Can't this be extracted out to the properties file? Looks like a hard coded config value.

Fixed in 698c71e

chamilad

Were you able to run any code coverage runs for the project? Do the unit tests cover important logic?

thusithathilina · 2017-04-19T03:30:43Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ApiUtility.java

+            int responseCode = httpResponse.getStatusLine().getStatusCode();
+            if (responseCode == 200) {
+                //success
+                bufferedReader = new BufferedReader(new InputStreamReader(httpResponse.getEntity().getContent(),


Can we use try with resource style for readers. Then you don't need to close them explicitly

Fixed in 698c71e

thusithathilina · 2017-04-19T03:31:22Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ApiUtility.java

+     * @throws CodeQualityMetricsException results
+     */
+    public static String callGraphQlApi(HttpPost httpPost) throws CodeQualityMetricsException {
+        BufferedReader bufferedReader = null;


If possible use java7 try with resource style

Fixed in 698c71e

thusithathilina · 2017-04-19T03:36:20Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/GithubApiCaller.java

+        }
+        return ApiUtility.callGraphQlApi(httpPost);
+    }
+}


why add call to al the methods? can we think of something like search, getHistory etc..

thusithathilina · 2017-04-19T03:36:43Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/PmtApiCaller.java

+        try {
+            Properties defaultProperties = new Properties();
+            ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
+            InputStream inputStream = classLoader.getResourceAsStream("url.properties");


Need to close the InputStreams

Fixed in 698c71e

thusithathilina · 2017-04-19T03:37:09Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/ReviewAnalyser.java

+            IssueApiResponse issueApiResponse = gson.fromJson(jsonText, IssueApiResponse.class);
+            issueApiResponse.getIssue().parallelStream()
+                    .filter(searchItem -> GITHUB_REVIEW_API_CLOSED_STATE.equals(searchItem.getStateOfThePr()))
+


remove blank line

Fixed in 698c71e

daneshk · 2017-04-19T04:00:37Z

org.wso2.codequalitymatrices/src/test/java/com/wso2/code/quality/metrics/ChangesFinderTest.java

+            result = IOUtils.toString(classLoader.getResourceAsStream(path));
+        } catch (IOException e) {
+            logger.debug(e.getMessage(), e.getCause());
+        }


why exception is ignored here. better throw the exception. if we need to ignore, log it as an error.

Fixed in 698c71e

daneshk · 2017-04-19T04:03:04Z

org.wso2.codequalitymatrices/src/main/java/com/wso2/code/quality/metrics/model/SearchItem.java

+ */
+public class SearchItem {
+    @SerializedName("state")
+    private String stateOfThePr;


better if we can change the variable name to prState

Fixed in 698c71e

close resources with try with resources Seperate tokens to a property file Fix bug from history simplificatio If two commits in the history of a file cancel each other out (the changes are reversed), then Git detect this and simplifies the git log output of the file by leaving out those two commits.

Shan1024 · 2017-04-21T06:40:09Z

README.md

+## Installation
+### Prerequisites
+* [Git](https://git-scm.com/book/en/v2/Getting-Started-Installing-Git) installed.
+* [Java](http://www.oracle.com/technetwork/java/javase/downloads/index-jsp-138363.html) installed


Missing . at the end.

Shan1024 · 2017-04-21T06:47:06Z

@kasunsiyambalapitiya : Please remove the org.wso2.codequalitymatrices directory from the project root as well(it would be better to have src, pom.xml, LICENSE and README in the project root). Please refer [1] for more details about the directory layout of a maven project.

[1] Introduction to the Standard Directory Layout

kasunsiyambalapitiya and others added 30 commits March 10, 2017 09:33

Correct the year in copyrights

72bb076

added .idea direcotory to gitignore file

2f68078

add entries for gitignore

3a24417

removed unwanted files from the repository

9b8be3f

Add log messages

2440d9f

Add log messages

9424e91

Add log messages for all classes

Merge branch used for adding logs to the classes

cfaa17f

Remove the CodeQualityMatrices.iml from remote

dda1a46

Get the patch details for a file made by the given commit

1c39bb0

Complete upto getting authors of the buggly lines from SDK

5699fb9

Added docs

c77dcd0

Pmt.java convert to use java8

df622e0

convert BlameCommit.java to java8

e3d9da5

create seperate class for calling graphql API

a9df118

Change the Logger initiliation in classes as suggested

9ee6d5f

in the pull request kasunsiyambalapitiya/CodeQualityMatricesProject#1 format the code

fix the copyright header

f4c2571

add CodeQualityMatricesException class

ebfef31

Add exceptions for classes

3c76e95

Remove invalid user input handling in RestApiCaller.java

0efc4b2

Rename log object and fixed possible null value passings

960a859

fix the changes asked by DilanUA in PR 2

57592c6

Changed method names to have a good English flow

bc8c7b8

change the part where it skips taking blme of new files

03c8563

replaced code with java 8 features

47e168e

added streams for saveReviewersToList method added streams for savePrNumberAndRepoName method replaced for loop with for each loop in readTheReviewOutJSON method

Replaced variables with contants

49c87dc

add constants for Reviewers.java

Correct Exceptional Handling

5fdedc4

Delete replaced codes from Java 8

43c52d1

fix formatting in java 8 stream API codes

772a100

Handle response codes

d28a361

Handle the response code recieved from Graphql API Correct the message thrown in Exception

fix getting graphql blame issue

81db43d

Shan1024 reviewed Apr 18, 2017

View reviewed changes

chamilad reviewed Apr 18, 2017

View reviewed changes

chamilad suggested changes Apr 18, 2017

View reviewed changes

thusithathilina reviewed Apr 19, 2017

View reviewed changes

daneshk reviewed Apr 19, 2017

View reviewed changes

kasunsiyambalapitiya changed the title ~~Modified to identify only the changed lines from a given patch~~ Modify to identify only the changed lines from a given patch Apr 19, 2017

kasunsiyambalapitiya added 2 commits April 20, 2017 10:36

Add README.md file

af853d8

Shan1024 reviewed Apr 21, 2017

View reviewed changes

kasunsiyambalapitiya added 2 commits April 21, 2017 18:44

Use Java Reflections

f50b3eb

Move Files outside

4ceb48f

kasunsiyambalapitiya force-pushed the newLogic branch from ad276dd to 4ceb48f Compare April 25, 2017 00:54

kasunsiyambalapitiya force-pushed the master branch from 8622a82 to 2c3e00e Compare September 4, 2020 12:00

Modify to identify only the changed lines from a given patch #2

Are you sure you want to change the base?

Modify to identify only the changed lines from a given patch #2

Conversation

kasunsiyambalapitiya commented Apr 18, 2017

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

kasunsiyambalapitiya Apr 20, 2017 • edited Loading

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

chamilad Apr 18, 2017 • edited Loading

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

chamilad Apr 18, 2017 • edited Loading

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

chamilad 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shan1024 commented Apr 21, 2017

kasunsiyambalapitiya Apr 20, 2017 •

edited

Loading

chamilad Apr 18, 2017 •

edited

Loading

chamilad Apr 18, 2017 •

edited

Loading