Skip to content

Bug 1695884 - Put PingType in a standalone file (#) #183

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2021
Merged

Bug 1695884 - Put PingType in a standalone file (#) #183

merged 1 commit into from
Apr 16, 2021

Conversation

@ChinYing-Li
Copy link
Contributor

@ChinYing-Li ChinYing-Li commented Apr 9, 2021

This PR aims to fix bug 1695884.
Any suggestion is appreciated!

ChinYing-Li referenced this pull request in ChinYing-Li/glean.js Apr 9, 2021
@ChinYing-Li
Bug 1695884 - Put PingType in a standalone file (#183)
02ced75 
Add entry to CHANGELOG.md
ChinYing-Li referenced this pull request in ChinYing-Li/glean.js Apr 9, 2021
@ChinYing-Li
Bug 1695884 - Put PingType in a standalone file (#183)
ac9635e 
Add entry to CHANGELOG.md
brizental
brizental suggested changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

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

Hey @ChinYing-Li thank you for your contribution. This is looking great. I left a few comments to be addressed in the code.

brizental
brizental suggested changes Apr 13, 2021
View reviewed changes
@ChinYing-Li
Copy link
Contributor Author

ChinYing-Li commented Apr 15, 2021

@brizental After rebasing, the PR is ready for review. Any suggestion is appreciated!
There's a conflict with main since I removed glean/src/core/pings/index.ts.

@brizental
Copy link
Contributor

brizental commented Apr 15, 2021

Changes look good, but you need to resolve the conflict before I can merge :)

@ChinYing-Li
Copy link
Contributor Author

ChinYing-Li commented Apr 16, 2021 •
edited
Loading

Closing this PR to open a clean one, as I don't have a quick solution to the conflicting index.ts; sorry for the inconvenience caused!

The commit history is clean now, and ready for review. Thanks!

@ChinYing-Li ChinYing-Li reopened this Apr 16, 2021
@brizental
Copy link
Contributor

brizental commented Apr 16, 2021

Looks great! Thanks for the hard work in cleaning up your commit history, great job :) Note that you didn't need to close the PR, force pushing would have been enough.

@brizental brizental merged commit d9e966c into mozilla:main Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@brizental brizental brizental approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ChinYing-Li @brizental