-
Notifications
You must be signed in to change notification settings - Fork 10
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 new syntax, fix credo #18
base: master
Are you sure you want to change the base?
Conversation
Don't merge just yet (if you even want to merge it), this is for discussion at this point. |
For note, everything should be piecemeal in different commits if you wish to review it more easily. And for note, I made this because I really hate elixir's normal I guess it might be ready to merge now if you think (and of course, if you want it). |
I am thinking of adding an option to return a tagged_tuple as well, if so should it be an option or another name for it ( |
Although I guess the user could just pipe it into the tagged tuple maker though... |
What is your thought on adding another expression type of Also, should I change Hmm, maybe |
Hmm, yeah I'm leaning to having |
Woah, didn't see ya there! Looking now 👀 |
You know, I like it! We'll probably see people abusing I'm generally a fan. Did you have any specific concerns, @OvermindDL1 ? |
{:blah, reason} -> %ErlangError{original: "Blah: #{reason}"} | ||
e -> e | ||
end | ||
block conversion_fun: conversion_fun do |
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.
👍
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.
I use the conversion function on occasion so I needed it exposed in at least the base method here. ^.^;
_ = 42 | ||
c <- {:blah, "Failed: #{b}"} | ||
c * 2 | ||
else |
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.
🤔 Hmm, I wonder if there's a more descriptive word than else
here? We have something preeeeetty close to try/rescue
here... perhaps rescue
, handle
, or errors do
?
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.
Well we can only use an Elixir block keyword if we want it block level here, which I really think we should for consistency with the rest of the language, so rescue
is possible, however I was thinking of leaving rescue
and catch
open for possible exception handling later too, so that really only left things like else
and after
and so forth, and as with rescue
/catch
, after
also has possible later use with message handling (I've envisioned possible receive
block enhancements in this as well later). So if you have any ideas?
end | ||
#=> 16 | ||
|
||
block do |
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.
Is there a more descriptive term than block
? handleable
? protected
? errorable
? Block is very general and could mean anything when reading through the code.
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.
I was actually paralyzed via Decision Paralysis for a long while so I chose the most simple and descriptive name, even if it is overly generic, and left it up to you if you wanted it changed. ^.^;
I did a search through a variety of libraries though and found it an unused function/macro name surprisingly, so it seemed worth grabbing. :-)
I'd personally think block
is overall fine though, it is a generic block that can be enhanced later with far more functionality if it is deemed useful, and renaming it later or having 20 slightly differently named variants for different purposes seems excessive to me. ^.^;
I can rename it though if you wish?
I'd maybe name them as separate block types or with options? We should be careful with overloading the tilde operators too much. There's only so many, and we want the code to remain clearish. I already feel bad introducing so many operators and in so many different packages 😬 So yeah, I'd leave the |
Mostly just what I mentioned above, which is pure usage issues, the code seems sound from my testing. :-)
That is what I was leaning to. It is pretty trivial to add more options to the block for more or special functionality and this seems a good, simple, unsurprising, and easy to use base. :-) |
This has languished for WAY TOO LONG. I need to dive back into this one; I'll have a but of time soon over the holiday break. Deepest apologies for how long this has taken 🙏 |
I wouldn't be remotely surprised if there were corner cases not caught so a good test suite would be very nice! |
I do apologize for combining a new syntax PR with an updated Credo PR, but fixing Credo 'as' I was making the new syntax was greatly useful since it stopped the many warnings I was getting on every-full-compile. ^.^;
In addition to the credo warning fixes this adds a new construct named
block
as well as an auto-raising version of it calledblock!
.The general pattern of
block
is like (copied from the docs added for it):