Skip to content

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 15, 2020

nimble test foo # error if no tests dir nor test task
nimble test --allowempty foo # no error if no tests dir nor test task

(refs #558 ; a warning is not enough, default should be error, but that's orthogonal to this PR)

/cc @dom96 the CI error seems mysterious and unrelated to this PR (and I'm also seeing it for another unrelated PR #781), do you know the source of this error?

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Err, what? Am I misunderstanding or are you making this change so that errors are ignored by Nimble just so you can get CI to work in Nim's repo? Why don't you remove the offending packages from the test suite instead (or stop nimble test being called for them)? Don't modify Nimble to ignore errors just because some packages are broken.

@dom96 dom96 closed this Mar 15, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Mar 15, 2020

Am I misunderstanding [...]
Don't modify Nimble to ignore errors just because some packages are broken.

They're already being ignored. As explained in nim-lang/Nim#13642 walkDir currently happily ignores errors, eg if dir is invalid/doesn't exist, it's a currently a noop instead of raising. So this PR #780 doesn't introduce any change in semantics, and also avoids breaking nim CI once nim-lang/Nim#13642 is merged.

/cc @dom96 maybe instead of immediately closing my PR, at least wait until I have a chance to respond to your comments?

Why don't you remove the offending packages from the test suite instead (or stop nimble test being called for them)?

  • that wouldn't help with the other change that was needed in this PR
  • it's the pre-existing behavior
  • I can also remove those 3 packages from nim PR if that's the preferred way (but again, the other change is needed)

@timotheecour timotheecour changed the title fix for https://github.com/nim-lang/Nim/pull/13642 [TODO] fix for https://github.com/nim-lang/Nim/pull/13642 Mar 15, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Mar 18, 2020

@dom96 could you please re-open?
I've disabled testing (via nimble test) the offending packages from the test suite in important_packages in nim-lang/Nim#13642, but this PR is still needed as explained in previous post. I can also undo the change in src/nimble.nim but that's introducing a change in behavior for nimble (unlike what I did in this PR which did not introduce a change in behavior as far as nimble is concerned)

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

Successfully merging this pull request may close these issues.

2 participants