Skip to content

Improve upgrade script to handle list comprehensions as arguments.#7229

Merged
rmlarsen merged 1 commit into
tensorflow:r1.0from
aselle:upgrade-4
Feb 3, 2017
Merged

Improve upgrade script to handle list comprehensions as arguments.#7229
rmlarsen merged 1 commit into
tensorflow:r1.0from
aselle:upgrade-4

Conversation

@aselle
Copy link
Copy Markdown
Contributor

@aselle aselle commented Feb 2, 2017

python's ast module does not return the correct location, so we
have to do our best to scan backwards to find where the [ token
that trully started the list comprehension occurs.

Copy link
Copy Markdown
Contributor

@annarev annarev left a comment

Choose a reason for hiding this comment

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

Can you add some tests with '[', spaces, and comments?

if comment_start == -1:
col = len(prev_line)
elif find_string_chars.search(prev_line[comment_start:]) is None:
col = comment_start
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the "col" be one of the return values?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tests are forthcoming (I used some to develop this that I haven't codified yet, but work), but I wanted to get this version up asap so that people wouldn't be hit by these problems. I'll put them in now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"col" here still looks unused to me. Is it expected?

Copy link
Copy Markdown
Contributor

@annarev annarev left a comment

Choose a reason for hiding this comment

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

Thank you for adding tests! The change looks good to approve only pending the question about "col" variable.

if comment_start == -1:
col = len(prev_line)
elif find_string_chars.search(prev_line[comment_start:]) is None:
col = comment_start
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"col" here still looks unused to me. Is it expected?

if __name__ == "__main__":
test_lib.main()

"""tf.concat(0, [tf.concat(0, tiles[y]) for y in range(4)])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you plan to use this comment later on? I am just making sure it is not a typo that you planned to remove.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

@rmlarsen rmlarsen added the stat:awaiting response Status - Awaiting response from author label Feb 3, 2017
python's ast module does not return the correct location, so we
have to do our best to scan backwards to find where the [ token
that trully started the list comprehension occurs.
@aselle
Copy link
Copy Markdown
Contributor Author

aselle commented Feb 3, 2017

I've added tests now.

@aselle aselle added awaiting review Pull request awaiting review and removed stat:awaiting response Status - Awaiting response from author labels Feb 3, 2017
@rmlarsen rmlarsen merged commit 114a462 into tensorflow:r1.0 Feb 3, 2017
@aselle aselle removed the awaiting review Pull request awaiting review label Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants