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: toolType to toolName #991

Closed
wants to merge 3 commits into from
Closed

fix: toolType to toolName #991

wants to merge 3 commits into from

Conversation

jlopes90
Copy link
Contributor

@jlopes90 jlopes90 commented Jun 28, 2019

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Fix

  • What is the current behavior? (You can also link to an open issue here)
    Measurement Added and Removed, toolType change to toolName.
    Because Modified and Completed is toolName.

  • What is the new behavior (if this is a feature change)?
    No

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No

  • Other information:
    No

@codecov
Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #991 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #991   +/-   ##
=======================================
  Coverage   13.05%   13.05%           
=======================================
  Files         223      223           
  Lines        7370     7370           
  Branches     1191     1191           
=======================================
  Hits          962      962           
  Misses       5402     5402           
  Partials     1006     1006
Impacted Files Coverage Δ
src/stateManagement/toolState.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d23c96...1ba1d4e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #991 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #991   +/-   ##
=======================================
  Coverage   13.11%   13.11%           
=======================================
  Files         223      223           
  Lines        7375     7375           
  Branches     1194     1194           
=======================================
  Hits          967      967           
  Misses       5400     5400           
  Partials     1008     1008
Impacted Files Coverage Δ
src/stateManagement/toolState.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 710ae6d...6485d4b. Read the comment docs.

@dannyrb dannyrb self-requested a review June 29, 2019 00:45

const eventType = EVENTS.MEASUREMENT_ADDED;
const eventData = {
toolType,
toolName,
Copy link
Member

@JamesAPetts JamesAPetts Jun 29, 2019

Choose a reason for hiding this comment

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

This is a breaking api change. I'm happy to change this as its technically correct, although we should have a deprecated alias with toolType Either that or we PR this to v4 only. What do you think @dannyrb ?

Copy link
Member

Choose a reason for hiding this comment

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

I think v4 is coming down the pipe soon enough that we should just do it there, and make a note of it in our breaking changes / migration guide.

@dannyrb
Copy link
Member

dannyrb commented Jul 11, 2019

@jlopes90 can you make this same change on our vNext branch?

@dannyrb dannyrb added the vNext label Jul 11, 2019
@jlopes90 jlopes90 closed this Jul 11, 2019
@jlopes90 jlopes90 deleted the fix-tool-type-to-name branch July 11, 2019 16:02
@jlopes90 jlopes90 restored the fix-tool-type-to-name branch July 11, 2019 16:03
@jlopes90 jlopes90 reopened this Jul 11, 2019
@jlopes90
Copy link
Contributor Author

jlopes90 commented Jul 11, 2019

@jlopes90 can you make this same change on our vNext branch?

At this moment, I'm busy, I have things to do with my project. So I'll see if I can.

@jlopes90
Copy link
Contributor Author

Sorry, I was wrong to click close pull request

@jlopes90
Copy link
Contributor Author

Can I change in vNext branch?

@JamesAPetts
Copy link
Member

@dannyrb What was the verdict on this? It flew over out radar for 4.0, and is still breaking as we discussed, shall we hotfix it in whilst 4.0 usage still isn't too mainstream (< 4 days old)?

@mikehazell
Copy link
Contributor

@dannyrb This can be closed now. #1216 fixed this issue.

@jlopes90 jlopes90 closed this Jul 15, 2020
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.

4 participants