-
Notifications
You must be signed in to change notification settings - Fork 224
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
refactor: clean up types slightly (drop GenericSpan, s/agent./apm./ namespace) #2300
Conversation
… docs - Drop the "GenericSpan" interface in our types. It isn't an external concept and there wasn't a lot of utility in using inheritance for it in the types file. - Change from 'agent.' to 'apm.' usage in the types test file because 'apm' is the prefix we encourage in docs (for good reason I think, it is less ambiguous).
💚 Build Succeeded
Expand to view the summary
Build stats
Trends 🧪 |
I'm considering the checks green. The failure of Integration Tests is the unrelated, ongoing elastic/apm-integration-testing#1188 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, with the caveat that all the use cases for these types aren't clear to me and I'll be curious to see if removing with type causes any weird downstream issues.
@@ -127,31 +127,27 @@ declare namespace apm { | |||
setSpanOutcome(outcome: Outcome): void; | |||
} | |||
|
|||
type Outcome = 'unknown' | 'success' | 'failure' | |||
type Outcome = 'unknown' | 'success' | 'failure'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary for this PR -- but seeing the added ;
makes me wonder if we want to get some sort of auto-fix linter looking at these typescript files as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, agreed it would be nice, but I didn't want to go there in this PR.
… docs (elastic#2300) - Drop the "GenericSpan" interface in our types. It isn't an external concept and there wasn't a lot of utility in using inheritance for it in the types file. - Change from 'agent.' to 'apm.' usage in the types test file because 'apm' is the prefix we encourage in docs (for good reason I think, it is less ambiguous).
concept and there wasn't a lot of utility in using inheritance for it
in the types file.
'apm' is the prefix we encourage in docs (for good reason I think, it
is less ambiguous).
This is a follow up to the types changes in #2119. I avoided this refactoring for that PR to keep the diff size down.
Checklist