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

fixes/8099 #8451

Closed
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@Bennyelg
Copy link

Bennyelg commented Jul 26, 2018

No description provided.

bennyelg
@@ -680,7 +680,7 @@ template applyIt*(varSeq, op: untyped) =
## var nums = @[1, 2, 3, 4]
## nums.applyIt(it * 3)
## assert nums[0] + nums[3] == 15
for i in 0 ..< varSeq.len:
for i in low(varSeq) ..< high(varSeq):

This comment has been minimized.

@SolitudeSF

SolitudeSF Jul 26, 2018

Contributor

shouldn't that be inclusive ..?

This comment has been minimized.

@Bennyelg
bennyelg
@Varriount
Copy link
Contributor

Varriount left a comment

Could you add a test for this fix?

@Bennyelg

This comment has been minimized.

Copy link

Bennyelg commented Jul 27, 2018

Sure, just add a test file in the tests dir?

bennyelg
@dom96

This comment has been minimized.

Copy link
Member

dom96 commented Jul 27, 2018

If you're changing the example then at least switch it to a runnableExamples.

@dom96

This comment has been minimized.

Copy link
Member

dom96 commented Jul 27, 2018

Also, for next time. Write "Fixes #8099" in your commit.

bennyelg
@Bennyelg

This comment has been minimized.

Copy link

Bennyelg commented Jul 27, 2018

@dom96
Sorry about the naming convention.
will do next time.
I'm trying to start to contribute where ever I can.

@@ -680,7 +680,15 @@ template applyIt*(varSeq, op: untyped) =
## var nums = @[1, 2, 3, 4]

This comment has been minimized.

@dom96

dom96 Jul 27, 2018

Member

This code block should also be moved into the runnable examples.

@dom96

This comment has been minimized.

Copy link
Member

dom96 commented Jul 27, 2018

@Bennyelg no worries.

bennyelg
@Bennyelg

This comment has been minimized.

Copy link

Bennyelg commented Jul 27, 2018

@dom96
anyIdea why it keeps failing on continuous integration ?

@skilchen

This comment has been minimized.

Copy link
Contributor

skilchen commented Jul 28, 2018

@Bennyelg you have hit a bug in Nim's handling of runnableExamples within templates. Nim simply generates invalid code for your example. I don't know if runnableExamples are supposed to work within templates. If yes this bug should be fixed, otherwise you have to move your test to one of the standard testament/tester tests probably best placed in the directory tests/seq or directly in tests/seq/tsequtils.nim

If you run nim doc sequtils you see what the problem is that travis complains about:

sequtils_examples.nim(6, 3) Error: '[]' cannot be assigned to

if you look at the produced sequtils_examples.nim you see:

block:
  var nums244444: array[1 .. 10, int]
  []=(nums244444, 1, 5)
  []=(nums244444, 10, 9)
  nums244444.applyIt(it + 1)
  doAssert nums244444[1] +
      nums244444[10] ==
      16
  doAssert nums244444[4] ==
      1

There are two misterious things here:

  1. Why does Nim not literally render your code as runnable code? (must have something to do with the fact that it is a runnableExample within a template.)
  2. Why is the prefix operator []= not correctly rendered as `[]=`?

The second point is the reason that your runnable example is uncompilable after this invalid transformation.

@Bennyelg

This comment has been minimized.

Copy link

Bennyelg commented Jul 28, 2018

@skellock

This comment has been minimized.

Copy link
Contributor

skellock commented Oct 3, 2018

#9143 sheds some light on the runnableExamples in templates issue. Once that closes, might be worth popping the stack up to this PR.

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 3, 2018

Gah, can you just PR the bugfix without runnableExamples then?

@Bennyelg

This comment has been minimized.

Copy link

Bennyelg commented Oct 3, 2018

@Araq runnableExamples Excluded.

## nums.applyIt(it * 3)
## assert nums[0] + nums[3] == 15
for i in 0 ..< varSeq.len:
## var setOfNums: array[1..10, int]

This comment has been minimized.

@dom96

dom96 Oct 4, 2018

Member

This will not be rendered correctly by the doc gen. Also, this example is far too long.

I'm going to cherry pick the fix out of this for faster turn-around.

dom96 added a commit that referenced this pull request Oct 4, 2018

@dom96 dom96 closed this Oct 4, 2018

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