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

Dataset Collection. Fixing problems with inlining #85

Merged
merged 35 commits into from
Nov 17, 2020
Merged

Conversation

Vitaly-Protasov
Copy link
Contributor

@Vitaly-Protasov Vitaly-Protasov commented Oct 27, 2020

Solved problem with commints in #50:

  • Problem to extract body of one-line methods: one-line several statements in a row
    public String getRemainingString() { return str.substring(index); }

  • Problem to inline methods with massive comments inside at the end of method declaration:

/*
        } else {
              pass
        }
*/
  • Problem with method invocation inside other statements: if (method_inv()). Added list of statements, being inside of which this problem can occure.

  • Problem with None bounds detection

@Vitaly-Protasov Vitaly-Protasov marked this pull request as draft October 27, 2020 11:03
@Vitaly-Protasov Vitaly-Protasov marked this pull request as ready for review October 27, 2020 15:54
@KatGarmash
Copy link
Member

@Vitaly-Protasov can you please pull the latest master branch?

@Vitaly-Protasov
Copy link
Contributor Author

@Vitaly-Protasov can you please pull the latest master branch?

pulled

Copy link
Contributor

@aravij aravij left a comment

Choose a reason for hiding this comment

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

@lyriccoder Are you capable now of this PR?

veniq/dataset_collection/augmentation.py Outdated Show resolved Hide resolved
@@ -75,6 +79,55 @@ def method_body_lines(method_node: ASTNode, file_path: Path) -> Tuple[int, int]:
return start_line, end_line


@typing.no_type_check
Copy link
Contributor

Choose a reason for hiding this comment

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

We better get rid of them.

Copy link
Member

Choose a reason for hiding this comment

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

fixed

@@ -75,6 +79,55 @@ def method_body_lines(method_node: ASTNode, file_path: Path) -> Tuple[int, int]:
return start_line, end_line


@typing.no_type_check
def check_attrs(
Copy link
Contributor

Choose a reason for hiding this comment

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

Those attrs are always exist for ASTNode. Is it this method where we want to distinguish from None? Then we should use Optional from typing.

Copy link
Member

Choose a reason for hiding this comment

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

fixed

ASTNodeType.SUPER_CONSTRUCTOR_INVOCATION,
ASTNodeType.TRY_STATEMENT
]
if check_attrs(method_invoked) and \
Copy link
Contributor

Choose a reason for hiding this comment

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

Nested ifs might be joined

Copy link
Member

Choose a reason for hiding this comment

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

rewritten

@@ -325,8 +325,7 @@ def get_lines_of_method_body(
line_with_declaration = lines[invocation_line - 1].split('=')
is_var_declaration = self.is_var_declaration(lines, invocation_line)
is_direct_return = self.is_direct_return(lines, invocation_line)
spaces_in_body = self.complement_spaces(body_start_line + 1, invocation_line, lines) * ' '

spaces_in_body = self.complement_spaces(body_start_line, invocation_line, lines) * ' '
Copy link
Contributor

Choose a reason for hiding this comment

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

We better multiply string by integer, not vice versa.

Copy link
Member

Choose a reason for hiding this comment

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

why is that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it is not that obvious what is happening here. If we will see string first, we will understand quicker that result is going to be another string. For now, one can consider that this expression is integer.

Copy link
Member

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@aravij aravij left a comment

Choose a reason for hiding this comment

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

Few comments. And you really better to use some formatter.

@@ -277,4 +277,4 @@ def percent_matched(dataset_range_lines, semi_range_lines):
total_case_handled = output_df.shape[0] - failed_cases_in_SEMI_algorithm - failed_cases_in_validation_examples
if total_case_handled > 0:
result = matched_cases / total_case_handled
print(f'Matched {result}% of cases, {matched_cases} out of {total_case_handled}')
print(f'Matched {result}% of cases, {matched_cases} out of {total_case_handled}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add line break in the end of the file.

@@ -153,6 +154,7 @@ def get_lines_after_invocation(
original_file = open(filename_in, encoding='utf-8')
lines = list(original_file)
lines_after_invoсation = lines[invocation_line:]
original_file.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want, you can use with ... as ... construction.

Copy link
Member

Choose a reason for hiding this comment

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

i just fixed the bug in some function which is not mine

except Exception:
continue
except Exception as e:
print(str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print(str(e))
print(e)

Copy link
Member

Choose a reason for hiding this comment

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

it's doesn't; work with JavaLang exceptions. only str(e) works

class_name=class_name,
method_node=method_node,
new_full_filename=new_full_filename,
original_func=original_func)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting formatting.

Suggested change
original_func=original_func)
original_func=original_func
)

test/dataset_collection/test_dataset_collection.py Outdated Show resolved Hide resolved
@@ -329,6 +344,21 @@ def test_inline_strange_body2(self):
open(test_filepath, encoding='utf-8') as test_ex:
self.assertEqual(actual_file.read(), test_ex.read())

def test_inline_comments_at_the_end(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some empty lines in the method body to break it into logical blocks for better understandability.

veniq/dataset_collection/augmentation.py Outdated Show resolved Hide resolved
ASTNodeType.SUPER_CONSTRUCTOR_INVOCATION,
ASTNodeType.TRY_STATEMENT
]
if method_invoked.parent is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is worth to join everything into a single if. If you gonna do this, you better add some boolean variables with meaningful name.

@lyriccoder
Copy link
Member

I fixed all valuable comments, the rest of them were subjective. I left them as it is

@lyriccoder lyriccoder merged commit a57dcae into master Nov 17, 2020
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