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

testament: error instead of silently ignore invalid targets; remove pointless alias target vs targets; document matrix; DRY #16343

Merged
merged 7 commits into from
Dec 14, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Dec 13, 2020

this will do the right thing:

XDG_CONFIG_HOME= nim r -f testament/testament.nim --targets:'js c' r tests/stdlib/thashes.nim

these will fail:

XDG_CONFIG_HOME= nim r -f testament/testament.nim --targets:'js c nonexistant' r tests/stdlib/thashes.nim

or after adding an invalid target in tests/stdlib/thashes.nim:

XDG_CONFIG_HOME= nim r -f testament/testament.nim r tests/stdlib/thashes.nim

@timotheecour timotheecour changed the title testament: error instead of silently ignore invalid targets testament: error instead of silently ignore invalid targets; remove pointless alias target vs targets Dec 13, 2020
@timotheecour timotheecour changed the title testament: error instead of silently ignore invalid targets; remove pointless alias target vs targets testament: error instead of silently ignore invalid targets; remove pointless alias target vs targets; document matrix Dec 13, 2020
@timotheecour timotheecour marked this pull request as ready for review December 14, 2020 01:19
@timotheecour timotheecour changed the title testament: error instead of silently ignore invalid targets; remove pointless alias target vs targets; document matrix testament: error instead of silently ignore invalid targets; remove pointless alias target vs targets; document matrix; DRY Dec 14, 2020
@Araq
Copy link
Member

Araq commented Dec 14, 2020

These aliases are not "useless", they let me get away with writing specs without constantly looking at the documentation. A world where "target" and "targets" are different is the really confusing world.

I'm merging this anyway, just trying to outline the ideas behind the testament aliases.

testament/specs.nim Outdated Show resolved Hide resolved
testament/specs.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Dec 14, 2020

Since testament is now an official tool, I added back the aliases "c++" and "target".

@timotheecour
Copy link
Member Author

timotheecour commented Dec 14, 2020

Since testament is now an official tool, I added back the aliases "c++" and "target".

ok for backward compatiblity argument (but probably a warning would be best in future work);

These aliases are not "useless", they let me get away with writing specs without constantly looking at the documentation. A world where "target" and "targets" are different is the really confusing world.

nim (stdlib etc) rightfully avoids this kind of aliasing usually, and testament seems to be an odd exception; aliases complicate other tasks such as grepping, and lead to people copying their favorite alias and making the problem worse.

I prefer a tool that is strict but also suggests spell corrections, and it's in fact easy to write a generic spellSuggest as a followup after #16067, via a completely generic case statement macro:

spellSuggest:
  case arg
  of "targets": ...
  of "output": ...
  else: raise newException(ValueError, spellCorrect)

which adds spell corrections automatically simply by invoking a generic macro spellSuggest:, and without having to add any logic to existing code beyond invoking spellSuggest:

output:"""
target:"c"
"""

=> invalid spec target: did you mean targets?

This would be generally applicable (nim cmdline etc)

@Araq Araq merged commit 7e1ae35 into nim-lang:devel Dec 14, 2020
@Araq
Copy link
Member

Araq commented Dec 14, 2020

invalid spec target: did you mean targets?
This would be generally applicable (nim cmdline etc)

I don't see this to work well for testament as I usually run batches of tests. However, TabNine is getting so good at this that we don't have to do anything here, not anymore.

@timotheecour timotheecour deleted the pr_specs_targets_cpp branch December 14, 2020 10:34
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
…ointless alias target vs targets; document matrix; DRY (nim-lang#16343)

* testament: error instead of silently ignore invalid targets
* s/target/targets/
* fix test; refs nim-lang#16344
* address comments
* Update testament/specs.nim

Co-authored-by: Andreas Rumpf <[email protected]>
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
…ointless alias target vs targets; document matrix; DRY (nim-lang#16343)

* testament: error instead of silently ignore invalid targets
* s/target/targets/
* fix test; refs nim-lang#16344
* address comments
* Update testament/specs.nim

Co-authored-by: Andreas Rumpf <[email protected]>
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.

3 participants