Add some comprehensions and set operations #59
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@Takashiidobe Thanks for proposing the PR, and sorry I changed some interfaces by the previous PR. Since most of the changes in this PR is implemented in the visitor, I think you can simply implement the same changes into the |
Takashiidobe
force-pushed
the
takashi/add-set-ops
branch
from
6390ed0
to
61cfc88
Compare
There was a problem hiding this comment.
Thanks! this change looks very helpful.
It also contains some issues (as I commented) and we will need some modification.
Since you said "Feel free to cherry pick the changes or modify in any way.", I'm considering to keep this pull request opened and prepare some other requests for the revised versions of this change.
| @@ -300,6 +315,25 @@ def visit_If(self, node, action): # pylint: disable=invalid-name | |||
| latex += self.visit(node) | |||
| return latex + r", & \mathrm{otherwise} \end{array} \right." | |||
|
|
|||
| def visit_SetComp(self, node, _): # pylint: disable=invalid-name | |||
| result = eval(compile(ast.Expression(node), '', 'eval')) | |||
There was a problem hiding this comment.
eval is not allowed due to its vulnerability. Since latexify accepts almost any code, this invocation may cause an arbitrary code execution. it may be better to adopt ast.literal_eval (it also allows a raw syntax tree).
Same for other parts.
| def visit_ListComp(self, node, _): # pylint: disable=invalid-name | ||
| result = eval(compile(ast.Expression(node), '', 'eval')) | ||
|
|
||
| sorted_result = sorted(result) |
There was a problem hiding this comment.
Especially for lists, this is not what the users wanted because lists are order-sensitive structure.
| left_type, right_type = type(node.left), type(node.right) | ||
| if left_type == right_type == ast.Set: |
There was a problem hiding this comment.
I think it is OK to go into the block if only either lhs or rhs is a Set.
| left_type, right_type = type(node.left), type(node.right) | |
| if left_type == right_type == ast.Set: | |
| if isinstance(left_type, ast.Set) or isinstance(right_type, ast.Set): |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """Tests for core.""" |
There was a problem hiding this comment.
| """Tests for core.""" | |
| """Tests for latexify.latexify_visitor.""" |
| @@ -0,0 +1,68 @@ | |||
| # Copyright 2020 Google LLC | |||
There was a problem hiding this comment.
This file looks to contain only end-to-end tests. It may be good to either:
- Move it into
src/integration_tests - Rewrite tests to directly call
LatexifyVisitor.
|
I'll leave this PR to you to cherry pick changes off of. I merged my changes locally to main but it seems to have caused a bunch of errors which I don't feel like fixing again. |
|
@Takashiidobe Thanks, I'll continue to work on this. |
As per #56, I tried adding some basic set operations and comprehensions. For comprehensions, I just evaled them, which might not be great (this introduces bugs if each element should be visited instead).
The Ast Nodes for comprehensions have some extra data that can be converted into something like${f(x) | x \in X}$ , but that can be improved in this PR or a later PR.
That being said, it kind of works, so I figured I'd put the changes up here. Feel free to cherry pick the changes or modify in any way.