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

tests: move proto roundtrip json to .tmp/ #10995

Merged
merged 2 commits into from
Jun 24, 2020
Merged

tests: move proto roundtrip json to .tmp/ #10995

merged 2 commits into from
Jun 24, 2020

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Jun 18, 2020

#6183 added this file and at the time it was tracked in source control

in #10557 we added the file to gitignore

now that it's ignored, seems like it can be in dist rather than nestled amongst tracked files.


i went with dist because #10994 but i suppose if we're not sharing artifacts between builds.... perhaps this makes more semantic sense in .tmp ?

@paulirish paulirish requested a review from a team as a code owner June 18, 2020 23:16
@paulirish paulirish requested review from patrickhulce and removed request for a team June 18, 2020 23:16
@paulirish paulirish changed the title test: move proto roundtrip json to dist tests: move proto roundtrip json to dist Jun 18, 2020
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

yeah this was an annoyance when I was relying on dependencies and artifacts during experimentation. great change.

although the above is not relevant any more so I agree doing .tmp makes more semantic sense. even if some other job needs that file ... i'd rather upload tmp as a separate artifact than use dist.

@patrickhulce
Copy link
Collaborator

I'll defer my review if @connorjclark has feelings here. I have no strong preference 🤷

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

another vote for .tmp (or wherever that's not dist/).

It shouldn't be a hard and fast rule, but it's nice to keep dist/ to things that are intended to actually be distributed (in one way or the other).

@paulirish paulirish changed the title tests: move proto roundtrip json to dist tests: move proto roundtrip json to .tmp/ Jun 23, 2020
@paulirish
Copy link
Member Author

.tmp done

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

Successfully merging this pull request may close these issues.

6 participants