Skip to content

Refactor compiler phases to avoid printing to std{out, err} - #777

Merged
rok-cesnovar merged 14 commits into
masterfrom
stancjs_no_prints
Dec 27, 2020
Merged

Refactor compiler phases to avoid printing to std{out, err}#777
rok-cesnovar merged 14 commits into
masterfrom
stancjs_no_prints

Conversation

@seantalts

@seantalts seantalts commented Dec 27, 2020

Copy link
Copy Markdown
Member

Fixes #741. @rok-cesnovar you were right, it was a larger refactor, lol. I got started and kept thinking it was almost done. I did a little unrelated cleanup as well and I'm proud to say this PR net deletes 200 lines (though mostly in repeated test code), heh.

Release Notes

Removed stdout and stderr in stanc.js.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@seantalts

Copy link
Copy Markdown
Member Author

I think with this in, we could do a bit more refactoring to adjust the filenames at print time and fix #716 in a not too bad way too...

@seantalts

seantalts commented Dec 27, 2020

Copy link
Copy Markdown
Member Author

(what I'm trying to do now is add an optional flag to pretty-printing warnings that will replace the filename just when pretty printed, but this will be for a new PR for 716)

@rok-cesnovar rok-cesnovar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks so much! This looks good to me.

Definitely a way to big refactor for me because it touches a bunch of fronted and the formatter. I was too lazy and opted for the easier route :)

@rok-cesnovar

Copy link
Copy Markdown
Member

I expanded the test to make sure someone doesnt reintroduce warnings printing for the current warnings. I pushed directly, hope you dont mind. If you think we can also fix what 716 is trying to that would be great. But can also merge as is.

@seantalts

Copy link
Copy Markdown
Member Author

Oh nice, good catch and test! Thanks and yep pushing directly is great. Seems like your computer and the Jenkins one (and mine) disagree about how node should print those long lines... any idea what's up with that? different versions of node maybe?

for now I will promote and then merge and keep working on a new PR for #716. It already has some cleanup of this one but I wanted to keep them split out into separate PRs to make them easier to review with a single intention for each one.

I've been wanting to do some refactoring in here for a while - it's way too obvious there were at least 3 very different authors in here, haha. Plus we had a big refactor on the MIR that was supposed to be extended to the AST but hasn't been yet, so those look completely different as well. Hopefully over time we can unify to one style!

@rok-cesnovar

Copy link
Copy Markdown
Member

any idea what's up with that? different versions of node maybe?

I would say so. Wrapped in a stringify to see if that would help.

@rok-cesnovar

Copy link
Copy Markdown
Member

It seems that wrapping in JSON.stringify helps. Will merge once tests pass.

@rok-cesnovar
rok-cesnovar merged commit 8188136 into master Dec 27, 2020
@rok-cesnovar
rok-cesnovar deleted the stancjs_no_prints branch December 27, 2020 16:32
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.

javascript parser chokes on #-style comments

2 participants