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

sets minor improvement #16087

Merged
merged 3 commits into from
Nov 21, 2020
Merged

sets minor improvement #16087

merged 3 commits into from
Nov 21, 2020

Conversation

ringabout
Copy link
Member

No description provided.

@timotheecour timotheecour merged commit f1764aa into nim-lang:devel Nov 21, 2020
@timotheecour
Copy link
Member

timotheecour commented Nov 21, 2020

  • carefully reviewed, merging.
    (was: assert => doAssert + a move)

  • note:
    these maybe(didn't check) correspond to builtin sets:
    tests/sets//t2669.nim
    tests/sets//tsets_various.nim
    tests/sets//tsets.nim

  • right now we have:

tests/collections/tcollections.nim
tests/collections/tcollections_to_string.nim
tests/collections/thashsets.nim
tests/collections/tseq.nim
tests/collections/ttables.nim
tests/collections/ttablesthreads.nim

given that the stdlib name is sets.nim (different from builtin sets), it'd be more logical to have
the test for sets.nim be under
tests/collections/tsets.nim
instead of
tests/collections/thashsets.nim

note that nim now allows multiple module with same name in same project, so
having these:
tests/collections/tsets.nim
tests/sets//tsets.nim

shouldn't cause conflicts even if merged via megatest.

@ringabout
Copy link
Member Author

If you see the commits before, you could find the CI failures due to name conflicts(tsets)

@timotheecour
Copy link
Member

timotheecour commented Nov 22, 2020

I can't find the logs? https://github.com/nim-lang/Nim/runs/1435593398
curious why it'd fail

but how about this: we could rename tests/sets//tsets.nim to tests/sets//tbuiltin_sets.nim and tests/collections/thashsets.nim to tests/collections/tsets.nim

@ringabout
Copy link
Member Author

tests/collections/thashsets.nim exists before, I'm just moving tests to them.

CI failures:

megatest.nim(910, 8) Error: redefinition of 'tsets'; previous declaration here: stdlib/tsets.nim(1, 2)

https://builds.sr.ht/~araq/job/348186

narimiran pushed a commit that referenced this pull request Nov 23, 2020
(cherry picked from commit f1764aa)
ringabout added a commit to ringabout/Nim that referenced this pull request Nov 25, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
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