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

fix #17853 (ascii message separator broke json nim dump) #17887

Merged

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 28, 2021

CI failure unrelated (classic flaky #17325)

@timotheecour timotheecour force-pushed the pr_fix_17853_unit_separator_optin branch 2 times, most recently from 3c38c24 to b7fade4 Compare April 28, 2021 23:32

var ps: seq[Process] # compile & run 2 progs in parallel
const nim = getCurrentCompilerExe()
Copy link
Member Author

Choose a reason for hiding this comment

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

previous code was using "nim", which didn't honor testament --nim:/pathto/nim r test

@timotheecour timotheecour marked this pull request as ready for review April 29, 2021 00:42
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Apr 29, 2021
@Araq
Copy link
Member

Araq commented Apr 29, 2021

I prefer to simply fix eg breaking nim dump --dump.format:json) instead. With your philosophy we keep adding more and more of these switches which then are covered by tests (which is good, don't misunderstand me!) -- however, it makes it harder and harder to contribute patches to the Nim compiler for non-experts. Eventually there is always some test failing somewhere for mysterious reasons. Complexity always bites back. And no, putting all these switches into a nice hierarchy does not help much.

@timotheecour timotheecour force-pushed the pr_fix_17853_unit_separator_optin branch from b7fade4 to c5b47c0 Compare April 29, 2021 08:28
@timotheecour timotheecour force-pushed the pr_fix_17853_unit_separator_optin branch from c5b47c0 to 18e78c6 Compare April 29, 2021 08:32
@timotheecour
Copy link
Member Author

timotheecour commented Apr 29, 2021

I prefer to simply fix eg breaking nim dump --dump.format:json) instead

done (but I can't help but remember the similar issues that dot processing caused)
I'm all for fewer options and sane defaults whenever possible, but sometimes, you need them, and when you do it helps that they're kept orthogonal, discoverable etc. In this case, keeping \31 without an off switch is fine i guess.

@timotheecour timotheecour changed the title fix #17853; make ascii message separator optin via --msgsep:on|off fix #17853 (ascii message separator broke json nim dump) Apr 29, 2021
@Araq Araq added merge_when_passes_CI mergeable once green and removed Ready For Review (please take another look): ready for next review round labels Apr 29, 2021
@timotheecour timotheecour merged commit 87229e2 into nim-lang:devel Apr 29, 2021
@timotheecour timotheecour deleted the pr_fix_17853_unit_separator_optin branch April 29, 2021 09:25
@timotheecour
Copy link
Member Author

see also #18162 which ended up doing --msgSep:on|off after all

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

Successfully merging this pull request may close these issues.

Compiler json dump inserts invalid json character at end of output
2 participants