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

Add a location to each of 'NIOHTTP2Errors' #247

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Sep 24, 2020

Motivation:

Without knowing where an error was thrown it can be unnecessarily
difficult to work out the reasons an application ended up in an error
state.

Modifications:

  • Add a 'file' string to each of the 'NIOHTTP2Errors' denoting the file
    and line where an error was created
  • Add static methods to create errors to 'NIOHTTP2Errors' defaulting
    file and line, this is unfortunately necessary as defaulting file and
    line in the init makes the defaulted and non-defaulted errors
    indistinguishable, and the compiler will pick the one with fewest
    defaulted values.
  • Add equatable conformance manually so as to ignore 'file'
  • Add backing storage to errors which no longer be stored in an existential
    container
  • Add 'CustomStringConvertible' conformance to the errors using backing
    storage so that the 'file' is actually visible when converting the
    error
  • Deprecate the old inits
  • Fix all deprecation warnings

Result:

  • NIOHTTP2Errors carry a 'file' with them indicating where they were
    created

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Sep 24, 2020
Copy link
Contributor

@Davidde94 Davidde94 left a comment

Choose a reason for hiding this comment

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

LGTM

@glbrntt glbrntt force-pushed the gb-error-locs branch 2 times, most recently from 9198cb3 to d23c0c2 Compare September 24, 2020 12:26
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks for doing this large chunk of work! I've left some notes in the diff, but nothing substantial.

Sources/NIOHTTP2/HTTP2ChannelHandler.swift Outdated Show resolved Hide resolved
Sources/NIOHTTP2/HTTP2Error.swift Show resolved Hide resolved
Sources/NIOHTTP2/HTTP2Error.swift Show resolved Hide resolved
@glbrntt glbrntt force-pushed the gb-error-locs branch 2 times, most recently from 8506ef8 to 0acd091 Compare September 24, 2020 13:43
Motivation:

Without knowing where an error was thrown it can be unnecessarily
difficult to work out the reasons an application ended up in an error
state.

Modifications:

- Add a 'file' string to each of the 'NIOHTTP2Errors' denoting the file
  and line where an error was created
- Add static methods to create errors to 'NIOHTTP2Errors' defaulting
  file and line, this is unfortunately necessary as defaulting file and
  line in the `init` makes the defaulted and non-defaulted errors
  indistinguishable, and the compiler will pick the one with fewest
  defaulted values.
- Add equatable conformance manually so as to ignore 'file'
- Add backing storage to errors which no longer be stored in an existential
  container
- Add 'CustomStringConvertible' conformance to the errors using backing
  storage so that the 'file' is actually visible when converting the
  error
- Deprecate the old `init`s
- Fix all deprecation warnings

Result:

- NIOHTTP2Errors carry a 'file' with them indicating where they were
  created
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one @glbrntt, good work!

@Lukasa Lukasa merged commit 440d64d into apple:master Sep 24, 2020
@glbrntt glbrntt deleted the gb-error-locs branch September 24, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants