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

case of variables in autolev parsing #16580

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

case of variables in autolev parsing #16580

wants to merge 2 commits into from

Conversation

vorabrijesh
Copy link

@vorabrijesh vorabrijesh commented Apr 7, 2019

Fixes #15166

References to other Issues or PRs

Brief description of what is fixed or changed

The output should not be in lower case
So, removed .lower() at some places and included lower_case=True

Other comments

please let me know which other places .lower() is to be removed if I have missed some places

Release Notes

  • parsing
    • removed .lower() at some places
    • added lower_case

@sympy-bot
Copy link

sympy-bot commented Apr 7, 2019

Hi, I am the SymPy bot (v145). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->
Fixes #15166
#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
The output should not be in lower case
So, removed `.lower() ` at some places and included `lower_case=True`

#### Other comments
please let me know which other places `.lower()` is to be removed if I have missed some places

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* parsing
  * removed `.lower()` at some places
  * added` lower_case`
<!-- END RELEASE NOTES -->

Copy link
Member

@Sc0rpi0n101 Sc0rpi0n101 left a comment

Choose a reason for hiding this comment

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

I found more than 150 hits for .lower() in _listener_autolev_antlr

Created a PR changing just the .lower() function in the file.

Also, the PR fails build tests for test_autolev
Please fix that

else:
name1 = ctx.ID().getText().lower() + str(i)
name1 = ctx.ID().getText() + str(i)
else:
name1 = ctx.ID().getText().lower()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name1 = ctx.ID().getText().lower()
name1 = ctx.ID().getText()

@vorabrijesh
Copy link
Author

@Sc0rpi0n101 I think we don't have to remove .lower() everywhere, only while outputting we have to remove it.

@Sc0rpi0n101
Copy link
Member

Wasn't the issue you referenced: #15166 state that you should avoid changing to lowercase at all?

Well, it's your patch. You can decide, if you don't want every change and only specific changes, I'll review my PR.

For now, focus on the build tests first. It is important to make sure the patch is working and doesn't break SymPy first.

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #16580 into master will decrease coverage by 0.133%.
The diff coverage is 1.639%.

@@             Coverage Diff              @@
##            master   #16580       +/-   ##
============================================
- Coverage   73.753%   73.62%   -0.134%     
============================================
  Files          619      619               
  Lines       158816   159308      +492     
  Branches     37247    37490      +243     
============================================
+ Hits        117133   117283      +150     
- Misses       36262    36599      +337     
- Partials      5421     5426        +5

Copy link
Member

@Sc0rpi0n101 Sc0rpi0n101 left a comment

Choose a reason for hiding this comment

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

Your patch has a syntax error.

Also, there will be many function calls for parse_autolev with only two arguments provided.
So, if you're introducing a new argument, you need to give it a default value whatever is being used now or change every function call.

sympy/parsing/autolev/__init__.py Outdated Show resolved Hide resolved
@vorabrijesh
Copy link
Author

can someone please review this pr?

@smichr
Copy link
Member

smichr commented Apr 12, 2019

The keyword is not implemented. Whether it is True or False, the same output is going to be generated, right?

@oscarbenjamin
Copy link
Contributor

From the OP above and the release note it's not clear to me what this PR does. Can you explain it more?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't change the case of autolev variables
6 participants