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

Interface stringification #317

Merged
merged 7 commits into from Apr 9, 2019
Merged

Interface stringification #317

merged 7 commits into from Apr 9, 2019

Conversation

ratulm
Copy link
Member

@ratulm ratulm commented Mar 27, 2019

No description provided.

@ratulm ratulm requested a review from progwriter March 27, 2019 17:24
@batfish-bot
Copy link

This change is Reviewable

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @progwriter and @ratulm)

a discussion (no related file):
@arifogel has some design for this that is intended to handle this and a whole slew of other issues. There's a huge amount of complication in doing this right; this PR should not go in on its own without that complexity accounted for.


@dhalperi dhalperi requested review from arifogel and removed request for progwriter March 27, 2019 18:19
@ratulm
Copy link
Member Author

ratulm commented Mar 28, 2019

@arifogel - is there a doc i can look at? else, lets chat offline.

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ratulm)


pybatfish/util.py, line 43 at r2 (raw file):

# Characters that must be escaped in name
# Should be in sync with SPECIAL_CHARS in CommonParser.java
_NAME_SPECIAL_CHARS_ = " \t,\\&()[]@" + "!#$%^;?<>={}"

Why is this split?

Copy link
Member Author

@ratulm ratulm left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @ratulm)


pybatfish/util.py, line 43 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Why is this split?

To exactly mirror the Java definition. (It is that way in Java because the first set of characters is being actively used and the second set is reserved for future use.)

@dhalperi
Copy link
Member

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

@arifogel has some design for this that is intended to handle this and a whole slew of other issues. There's a huge amount of complication in doing this right; this PR should not go in on its own without that complexity accounted for.

Unblocking myself now that you're talking :)


Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@arifogel arifogel merged commit 4ef640b into master Apr 9, 2019
@arifogel arifogel deleted the interface-stringification branch April 9, 2019 17:15
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.

None yet

4 participants