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

cleanup old codes #16173

Merged
merged 1 commit into from
Nov 30, 2020
Merged

cleanup old codes #16173

merged 1 commit into from
Nov 30, 2020

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 28, 2020

ref timotheecour#389

clean when (NimMajor, NimMinor, NimPatch) > (0, 10, 2): and unnecessary imports.

@Clyybber
Copy link
Contributor

Clyybber commented Nov 28, 2020

Should be left as is IMO, it might test some otherwise untested path and the compiler should continue to be correct for non-minimized/cleaned-up code too, afterall :)

@timotheecour
Copy link
Member

timotheecour commented Nov 30, 2020

Should be left as is IMO, it might test some otherwise untested path and the compiler should continue to be correct for non-minimized/cleaned-up code too, afterall :)

This is historical artifcat; tests should have a specific purpose; if you want that, put this logic in a dedicated code block whose intent is to test that. And there already are plenty of tests that test this (eg when false ... else: ...)

@ringabout ringabout merged commit 3589631 into nim-lang:devel Nov 30, 2020
@Araq
Copy link
Member

Araq commented Nov 30, 2020

So where is this other test that does a Nim version check inside a concept? I'm leaving it as it is for now but please be a little bit more careful. Tests shouldn't be "minimal" or beautiful, tests can never test enough!

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.

4 participants