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

Issue #4641: Avoid usage of getLines method from FileText #4670

Merged
merged 1 commit into from
Jul 10, 2017

Conversation

timurt
Copy link
Contributor

@timurt timurt commented Jul 9, 2017

@codecov-io
Copy link

codecov-io commented Jul 9, 2017

Codecov Report

Merging #4670 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4670   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         284     284           
  Lines       15367   15372    +5     
  Branches     3499    3500    +1     
======================================
+ Hits        15367   15372    +5
Impacted Files Coverage Δ
.../com/puppycrawl/tools/checkstyle/api/FileText.java 100% <ø> (ø) ⬆️
...ls/checkstyle/checks/header/RegexpHeaderCheck.java 100% <100%> (ø) ⬆️
...wl/tools/checkstyle/checks/header/HeaderCheck.java 100% <100%> (ø) ⬆️
...s/checkstyle/checks/regexp/SinglelineDetector.java 100% <100%> (ø) ⬆️
...style/checks/whitespace/FileTabCharacterCheck.java 100% <100%> (ø) ⬆️
...tools/checkstyle/checks/sizes/FileLengthCheck.java 100% <100%> (ø) ⬆️
...tools/checkstyle/checks/UniquePropertiesCheck.java 100% <100%> (ø) ⬆️
...heckstyle/checks/regexp/RegexpSinglelineCheck.java 100% <100%> (ø) ⬆️
.../checkstyle/checks/imports/ImportControlCheck.java 100% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc633b...a461e97. Read the comment docs.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

@timurt , please launch diff regression testing on affected Checks, no diff is expected.

few items to improve:

* @return line number of first occurrence. If no key found in properties
* file, 0 is returned
*/
protected static int getLineNumber(FileText fileText, String keyName) {
Copy link
Member

Choose a reason for hiding this comment

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

ideally it should be private, but will need it in Tests to avoid even deeper refactoring.
I will create separate issue on UT refactoring, it could be done later.

Copy link
Member

Choose a reason for hiding this comment

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

Issue #4694

@@ -104,9 +103,7 @@ protected void processFiltered(File file, FileText fileText) {
* file, 0 is returned
*/
protected static int getLineNumber(List<String> lines, String keyName) {
Copy link
Member

Choose a reason for hiding this comment

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

this method is not in use in Check so should be removed. Looks like it is required by Tests, Tests need to use new method.

Copy link
Member

Choose a reason for hiding this comment

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

Issue #4694

@timurt timurt force-pushed the 4641 branch 2 times, most recently from 9e55814 to 3711642 Compare July 9, 2017 19:04
* method return value.
*/
@Test
public void testNotFoundKey() {
public void testNotFoundKeyFileText() {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need FileText on the end of test name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

* Processes a set of lines looking for matches.
* @param fileText {@link FileText} object contains the lines to process.
*/
public void processLines(FileText fileText) {
Copy link
Member

Choose a reason for hiding this comment

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

There is also a method public void processLines(List<String> lines).
Why is the other method remaining? Shouldn't it be swapped for this new method instead of keeping both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right, done

@timurt
Copy link
Contributor Author

timurt commented Jul 10, 2017

@romani
Here is my regression
https://timurt.github.io/4641/

@romani romani merged commit 0c2abad into checkstyle:master Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants