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

[WIP]Feature/issue 660 #666

Closed
wants to merge 4 commits into from

Conversation

shes50103
Copy link
Member

@shes50103 shes50103 commented May 13, 2018

  • Implement underscore feature
  • make sure underscore using in the right way

1_2_3 => 123
1_2.3_4 => 12.34
1__2 => Error
_1 => Error
1_ => Error

This closes #660

@ghost ghost assigned shes50103 May 13, 2018
@ghost ghost added the in progress label May 13, 2018
@codecov
Copy link

codecov bot commented May 13, 2018

Codecov Report

Merging #666 into master will decrease coverage by 0.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
- Coverage   82.94%   82.93%   -0.02%     
==========================================
  Files          56       56              
  Lines        7501     7507       +6     
==========================================
+ Hits         6222     6226       +4     
- Misses       1027     1028       +1     
- Partials      252      253       +1
Impacted Files Coverage Δ
compiler/lexer/lexer.go 94.7% <77.77%> (-0.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c52b53e...98de941. Read the comment docs.

@st0012 st0012 added this to the version 0.1.10 milestone May 14, 2018
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I think it's worth to mention how the wrong cases would behave in Ruby. Like 1_ in Ruby will do nothing instead of raising an error


for isDigit(l.ch) || l.ch == '_' {

if isDigit(l.ch) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO I think the lexer should preserve the underscores in the token. In terms of responsibility, I don't think a lexer should be responsible for any code transformation. Removing them at this stage seems sort of magical.

IMO the token should be simplified at a later stage, maybe during parsing or eval?

@st0012
Copy link
Member

st0012 commented May 14, 2018

Well I think @SD10 has a good point. Keeping those information to parser allows us to provide more sophisticated error message

@SD10
Copy link
Member

SD10 commented May 14, 2018

Here we are already transforming the token into a literal for Int:

func (p *Parser) parseIntegerLiteral() ast.Expression {

Here for Float:
func (p *Parser) parseFloatLiteral(integerPart ast.Expression) ast.Expression {

I think this is a good place to do the transformation work.

This is just my opinion. I'm a newbie 😊

@SD10
Copy link
Member

SD10 commented May 14, 2018

It's also worth noting that in the Ruby lexer the underscores are maintained:

...
[[2, 7], :on_int, "1_000_000"],

@shes50103
Copy link
Member Author

I will preserve the underscores in the token and handle it in parser,
in ruby irb 1_ this case will raise error at the next input so I will still remain this feature

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.

underscore in numbers
3 participants