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.

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

Add interface to require dead_end without core_ext #119

Merged
merged 4 commits into from
Nov 21, 2021
Merged

Add interface to require dead_end without core_ext #119

merged 4 commits into from
Nov 21, 2021

Conversation

schneems
Copy link
Collaborator

@schneems schneems commented Nov 18, 2021 •
edited
Loading

Currently dead_end works by monkey patching require and friends.

If someone wants to programmatically execute dead_end manually via DeadEnd.handle_error without monkey patches they can now do that by setting the environment variable DISABLE_DEAD_END_CORE_EXT=1.

If someone wants to programmatically execute dead_end manually via DeadEnd.handle_error without monkey patches they can now do that by requiring dead_end/api.

I'm specifically interested in feedback around interfaces as they're harder to change after they're adopted.

  • Any behavior described in DeadEnd.handle_error sound weird or confusing?
  • Disabling monkeypatches via an env var a decent enough interface?
  • Env var name okay?

@schneems schneems force-pushed the schneems/no-core-ext branch 2 times, most recently from ef9f844 to 1f2541a Compare November 18, 2021 22:27
@schneems
Add interface to require dead_end without core_ext
56d2bf0 
Currently `dead_end` works by monkey patching `require` and friends.

If someone wants to programmatically execute dead_end manually via `DeadEnd.handle_error` without monkey patches they can now do that by setting the environment variable `DISABLE_DEAD_END_CORE_EXT=1`.
@jez
Copy link
Contributor

jez commented Nov 19, 2021 •
edited
Loading

Instead of an environment variable, which people could forget to set in certain environments (laptop, dev box, CI, QA, production, etc.), what do you think about factoring most of the contents of lib/dead_end.rb into a file called like lib/dead_end/api.rb, and making the contents of lib/dead_end.rb be basically:

require_relative 'dead_end/api'
require_relative 'dead_end/core_ext'

?

That wouldn't break any existing users, and would also allow people who want to avoid the monkey patch to just do

require 'dead_end/api'

and never have to worry about making sure the environment variable is set. This is similar to how some other gems work.

Curious what you think!

@schneems
Copy link
Collaborator Author

Previously I had all the code in lib/internals.rb (and the interfaces were dead_end/fyi and dead_end/auto). The interface was annoying to work with for development as I could never remember what I named the main file. I love the suggestion of api though. It is short and memorable. I'll re-work the PR. Thanks for taking a look.

@schneems
Move api to dead_end/api instead of env var
dc18d1a 
Because setting environment variables on systems may be difficult or inconsistent we are moving the API to be require based. Now anyone who wants to use dead_end without mutating `require` can do so via requiring `dead_end/api`.
@schneems
Copy link
Collaborator Author

Updated the PR to move to require 'dead_end/api' as the interface. If you can give it a last look-through I can merge and ship this in 3.1.0. Thanks for the suggestion!

jez
jez approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@jez jez left a comment

Choose a reason for hiding this comment

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

lgtm thanks!

lib/dead_end/api.rb Outdated

# DeadEnd.handle_error [Public]
#
# Takes an exception from a syntax error, uses that
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the interface specifically that callers must pass a SyntaxError exception? I see that you don't mention that in the rescue line below, though you do mention it in the lib/dead_end/core_ext.rb file.

If it's intended for that requirement to be explicit, do you want to check that in handle_error like

  raise TypeError.new("must pass a SyntaxError") unless e.is_a?(SyntaxError)

(I find that it's usually more forgiving to start with a narrow API and widen it, but it's up to you.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a little torn. I agree that being stricter is better. Rather than raising a new error though I decided to warn and re-raise the original error which is a pattern I have in other places.

Take a look and let me know what you think. Also as a heads up, if you all haven't run into it yet require_relative monkeypatching is broken with Ruby 3.1 and needs to be fixed Shopify/bootsnap@0d64e7e

Copy link
Contributor

Choose a reason for hiding this comment

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

works for me.

lib/dead_end/api.rb Outdated Show resolved Hide resolved
@schneems @jez
Update lib/dead_end/api.rb
78297ee 
Co-authored-by: Jake Zimmerman <zimmerman.jake@gmail.com>
schneems added a commit that referenced this pull request Nov 20, 2021
@schneems
Add explicit check for Syntax Error
34d1dde 
As suggested in #119 (comment) we are now explicitly checking the type of the incoming error. Rather than raise a new error, I've chosen to emit a warning and re-raise the original error. This is a similar pattern to the case where we are not able to detect the filename from the error message. The goal is to provide visibility into the source of the behavior, but to break as few expectations as possible. 

The `DeadEnd.handle_error` should (ideally) never raise an error to the end user. The purpose of the library is to provide progressive error enhancement. If it fails it means we cannot make a better error, so therefore we should warn (so the user can fix or report the problem) and then fall back to the original error.

I also added a test for the failure error condition where the filename cannot be pulled out of the SyntaxError message which was added in #114.
@schneems
Add explicit check for Syntax Error
0b40746 
As suggested in #119 (comment) we are now explicitly checking the type of the incoming error. Rather than raise a new error, I've chosen to emit a warning and re-raise the original error. This is a similar pattern to the case where we are not able to detect the filename from the error message. The goal is to provide visibility into the source of the behavior, but to break as few expectations as possible. 

The `DeadEnd.handle_error` should (ideally) never raise an error to the end user. The purpose of the library is to provide progressive error enhancement. If it fails it means we cannot make a better error, so therefore we should warn (so the user can fix or report the problem) and then fall back to the original error.

I also added a test for the failure error condition where the filename cannot be pulled out of the SyntaxError message which was added in #114.
@schneems schneems merged commit c4ce262 into main Nov 21, 2021
@schneems schneems deleted the schneems/no-core-ext branch November 21, 2021 13:27
@schneems
Copy link
Collaborator Author

DeadEnd 3.1.0 is released with this change 🎉 🎉 🎉 Thanks for all the feedback ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jez jez jez approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schneems @jez