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

Automatically create output folders #252

Merged
merged 4 commits into from
Dec 12, 2018
Merged

Conversation

lorenz
Copy link
Contributor

@lorenz lorenz commented Dec 4, 2018

This is an implementation of google/jsonnet#195 for the Go version since the concerns mentioned in that issue are of no concern in a Go application. It's just a very quick fix done for my own needs, feel free to reject in case you don't want any divergence from the C++ implementation.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@coveralls
Copy link

coveralls commented Dec 4, 2018

Coverage Status

Coverage increased (+3.1%) to 75.14% when pulling 29b520a on lorenz:master into 9a9954a on google:master.

@lorenz
Copy link
Contributor Author

lorenz commented Dec 4, 2018

Signed the CLA

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 4, 2018
@sparkprime
Copy link
Contributor

This is great but can you put it in a flag that's off by default and also make it do it in the single file case?

@lorenz
Copy link
Contributor Author

lorenz commented Dec 4, 2018

Sure

@lorenz
Copy link
Contributor Author

lorenz commented Dec 4, 2018

Made it into an option and added single file support

@lorenz
Copy link
Contributor Author

lorenz commented Dec 6, 2018

The failed tests are in the cpp-jsonnet repo, how should this be synchronized? If the PR there goes in before this one, the build is broken and the other way round too.

@sbarzowski
Copy link
Collaborator

Oh, we're comparing help output directly. Well this is our test mechanism's fault, sorry about that. It's not very important how we go around it, as long as we update both in the timespan of less than few hours. Pehaps later we can move implementation-specific output to implementation repo (it should be just a few lines of changes in the scripts).

@sparkprime
Copy link
Contributor

I believe it should be possible to update cpp-jsonnet but not go-jsonnet, and then go-jsonnet will not take that version of cpp-jsonnet until it is updated, so you can do that update at the same time as introducing the change.

@lorenz
Copy link
Contributor Author

lorenz commented Dec 7, 2018

Update seems to have broken another assertion (non cmd-related), not sure about that

@sbarzowski
Copy link
Collaborator

I don't think it has anything to do with this change. It's happened just because of the submodule update. I'll investigate later today.

@sbarzowski
Copy link
Collaborator

As I thought, it's just the matter of updating stdlib to match C++ updates (there was a bugfix related to escaping there). If you add this commit to the pull request the tests should run fine sbarzowski@baff839

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@lorenz
Copy link
Contributor Author

lorenz commented Dec 10, 2018

Looks good now

@sbarzowski
Copy link
Collaborator

Yes, I'm officially ok with my commit being merged here.

@sparkprime
Copy link
Contributor

Thanks

@sparkprime sparkprime merged commit 7012604 into google:master Dec 12, 2018
@alem0lars
Copy link

Is this feature released?

@sbarzowski
Copy link
Collaborator

Yes

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

Successfully merging this pull request may close these issues.

6 participants