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

Change regex to return Result #380

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@eeue56
Contributor

eeue56 commented Aug 30, 2015

Changed the Regex.regex function to return a Result as per #378 and updated and added tests to this effect. I haven't changed any of the documentation for any other function in Regex.elm, as I feel that rtfeldman's comment on providing a built-in regex syntax needs to be discussed further - certainly it makes code more bulky and there is no way to create a Regex without the regex function.

@eeue56 eeue56 referenced this pull request Aug 30, 2015

Closed

Regex, runtime error #378

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jan 2, 2016

Member

The more I think about it, the more I like this idea. You can use it to implement the current behavior by using Debug.crash to handle the Err case, which could be offered as a convenience function like regexOrCrash

Member

rtfeldman commented Jan 2, 2016

The more I think about it, the more I like this idea. You can use it to implement the current behavior by using Debug.crash to handle the Err case, which could be offered as a convenience function like regexOrCrash

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Jan 2, 2016

Contributor

Yeah, that's true. I like the Debug.crash approach

Contributor

eeue56 commented Jan 2, 2016

Yeah, that's true. I like the Debug.crash approach

@robertjlooby

This comment has been minimized.

Show comment
Hide comment
@robertjlooby

robertjlooby Jun 15, 2016

Contributor

Has any more thought been given to making this change?

Contributor

robertjlooby commented Jun 15, 2016

Has any more thought been given to making this change?

try
{
compiledRegex = new RegExp(raw, 'g');

This comment has been minimized.

@tmcw

tmcw Jul 11, 2016

minor nitpick that you could express this more succinctly as

return Result.Ok(new RegExp(raw, 'g'));

Here, and avoid using the temporary variable - the code would have the same behavior since new RegExp(raw, 'g') is evaluated first, would throw, and not instantiate a Result.Ok

@tmcw

tmcw Jul 11, 2016

minor nitpick that you could express this more succinctly as

return Result.Ok(new RegExp(raw, 'g'));

Here, and avoid using the temporary variable - the code would have the same behavior since new RegExp(raw, 'g') is evaluated first, would throw, and not instantiate a Result.Ok

@tmcw

This comment has been minimized.

Show comment
Hide comment
@tmcw

tmcw Jul 11, 2016

Yep, as I posted on the other ticket, I 2nd this approach as flexible and lower-impact than a syntax extension.

tmcw commented Jul 11, 2016

Yep, as I posted on the other ticket, I 2nd this approach as flexible and lower-impact than a syntax extension.

@seanhess

This comment has been minimized.

Show comment
Hide comment
@seanhess

seanhess Sep 10, 2016

This would be very helpful to me for the project I am currently working on. Right now we are blocked without it.

seanhess commented Sep 10, 2016

This would be very helpful to me for the project I am currently working on. Right now we are blocked without it.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Sep 10, 2016

Contributor

@seanhess here's how to unblock you:

  • Create a function that looks like this:
var isValidRegex = function(regex, modifiers){
  try {
    new RegExp(regex, modifiers);
  } catch (e) {
    return [regex, false];
  }

  return [regex, true];
}
  • Make a port that looks like validateRegex : Cmd String
  • Make a port that looks like isValidRegex : Sub (String, Bool)

If you really want, it's trivial to introduce this as a native function into your project. Don't let yourself get blocked on this! it's a trivial issue to solve :)

Contributor

eeue56 commented Sep 10, 2016

@seanhess here's how to unblock you:

  • Create a function that looks like this:
var isValidRegex = function(regex, modifiers){
  try {
    new RegExp(regex, modifiers);
  } catch (e) {
    return [regex, false];
  }

  return [regex, true];
}
  • Make a port that looks like validateRegex : Cmd String
  • Make a port that looks like isValidRegex : Sub (String, Bool)

If you really want, it's trivial to introduce this as a native function into your project. Don't let yourself get blocked on this! it's a trivial issue to solve :)

@seanhess

This comment has been minimized.

Show comment
Hide comment
@seanhess

seanhess Sep 19, 2016

@eeue56 ssh, you're removing the imperative to get this merged ;D

Thanks though, that's a great workaround! I forget that native modules are an option for end applications since they can't be published.

seanhess commented Sep 19, 2016

@eeue56 ssh, you're removing the imperative to get this merged ;D

Thanks though, that's a great workaround! I forget that native modules are an option for end applications since they can't be published.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 22, 2016

Member

Tracking in #722. These changes are quite easy, so I don't think a PR is needed. Just need to figure out the appropriate time to actually make a change like this.

Member

evancz commented Sep 22, 2016

Tracking in #722. These changes are quite easy, so I don't think a PR is needed. Just need to figure out the appropriate time to actually make a change like this.

@evancz evancz closed this Sep 22, 2016

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