Skip to content

Conversation

@i4ki
Copy link
Collaborator

@i4ki i4ki commented Mar 29, 2017

Closes #188

Signed-off-by: Tiago Natel de Moura <tiago.natel@neoway.com.br>
@i4ki i4ki requested review from katcipis and vitorarins March 29, 2017 04:58
Signed-off-by: Tiago Natel de Moura <tiago.natel@neoway.com.br>
@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

Merging #195 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage    55.9%   55.91%   +0.01%     
==========================================
  Files          22       22              
  Lines        3998     3999       +1     
==========================================
+ Hits         2235     2236       +1     
  Misses       1519     1519              
  Partials      244      244
Impacted Files Coverage Δ
scanner/lex.go 85.89% <100%> (+0.13%) ⬆️
internal/sh/rfork_linux.go 76.19% <0%> (-0.32%) ⬇️

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 3776d9c...aab08e2. Read the comment docs.

Copy link
Member

@vitorarins vitorarins left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@katcipis katcipis left a comment

Choose a reason for hiding this comment

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

I would love to start to see at least one full integration test for this kind of thing. The internal tests that are developed are quite hard to understand (at least to someone as limited as me ).

Besides that, LGTM

@i4ki
Copy link
Collaborator Author

i4ki commented Apr 3, 2017

@katcipis by full integrated test you mean adding tests in the interpreter (internal/sh) also?

@katcipis
Copy link
Member

katcipis commented Apr 3, 2017

@tiago4orion actually I was even thinking on testing through the public sh API (the same one we used on klb tests). Less likely to break and gives a code example of what is being done/fixed :-)

@vitorarins
Copy link
Member

@tiago4orion is this PR still relevant? Can we merge?

@i4ki
Copy link
Collaborator Author

i4ki commented Jun 6, 2017

missing some tests...

@i4ki
Copy link
Collaborator Author

i4ki commented Jun 6, 2017

tests are too cryptic.. I'll improve that soon

Signed-off-by: Tiago Natel de Moura <tiago.natel@neoway.com.br>
// $HOME+ # used in: $HOME+"/src"
// $HOME] # used in: $list[$index]
// $HOME) # used in: call($HOME)
if !isValidVariableSuffix(next) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice refactoring. You sir are a gentleman:

gentleman

@katcipis
Copy link
Member

katcipis commented Mar 27, 2019

@tiago4orion this will sound a little repetitive with what @vitorarins said, but can we merge this ? =P If it is important we can add better tests later =) (at least it does not break anything and the problem seems serious)

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.

4 participants