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

Update ProcessMonitor errors to contain error messages #3532

Merged
merged 3 commits into from
May 1, 2020

Conversation

chalcolith
Copy link
Member

This is a breaking change, as the ProcessError types are now classes, not primitives, and have a .string() method to return their error messages.

In addition, there is no more Unsupported error type.

This is a breaking change, as the `ProcessError` types are now classes, not primitives, and have a `.string()` method to return their error messages.

In addition, there is no more `Unsupported` error type.
@chalcolith chalcolith added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label May 1, 2020
@SeanTAllen
Copy link
Member

@kulibali I dont think these should be classes.

They should stay as primitives but primitives with a "string" function that returns an error message. The string representation of the error.

@chalcolith
Copy link
Member Author

I thought it would be useful to be able to be able to include, e.g. particular filenames (or Windows error messages) in the strings.

Do you think this is not worth it?

@SeanTAllen
Copy link
Member

@kulibali I'm not sure of the utility right now based on what I know about the usage.

@chalcolith
Copy link
Member Author

I would find it quite useful for debugging my Windows updates to corral, stable, and ponyup.

@SeanTAllen
Copy link
Member

@kulibali perhaps a mix of class and primitive? Do they all need a custom message, for corral, its a string that was just "blah" basically for a string representation of the name. I suppose classes don't really matter much as we won't be allocating a lot of them.

@chalcolith chalcolith mentioned this pull request May 1, 2020
@SeanTAllen
Copy link
Member

@kulibali thinking about it some more. if it is going to be classes. I think a generic error class that contains both a possible custom error and the primitive for type of error would be best with each primitive having its own default string representation.

Then it could be something like (this shortened version)

class ProcessError
  let error_type: ProcessErrorType
  let message: String

fun string(): String =>
  prim.string() + ": " + message

(but more allocation friendly) for string().

Thoughts?

@chalcolith chalcolith added the do not merge This PR should not be merged at this time label May 1, 2020
@chalcolith chalcolith removed the do not merge This PR should not be merged at this time label May 1, 2020
| CapError => _env.out.print("ProcessError: CapError")
else _env.out.print("Unknown ProcessError!")
end
_env.out.print(err.string())
Copy link
Contributor

Choose a reason for hiding this comment

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

😻


fun string(): String =>
match message
| let m: String => error_type.string() + ": " + m
Copy link
Contributor

@mfelsche mfelsche May 1, 2020

Choose a reason for hiding this comment

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

Having the string() function return a String val might be confusing, as Stringable in stdlib is defined as returning a String iso^. I would also make the ProcessErrorType primitives return a String iso^ to be in line with Stringable as well.

May I suggest to use sth like this in this line here?

let err_type_name = error_type.string()
let msg = 
  recover iso 
    let tmp = String(err_type_name.size() + 2 + m.size())
    tmp.>append(consume err_type_name)
    .>append(": ")
    .>append(m)
end

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that the returned string should not be mutable.

Copy link
Member

Choose a reason for hiding this comment

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

I think String iso^ is preferable so it lines up with Stringable.

Copy link
Member

Choose a reason for hiding this comment

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

Being Stringable was my intent when I suggested the idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, updated.

@chalcolith chalcolith merged commit 47fcba3 into master May 1, 2020
@chalcolith chalcolith deleted the process_error_messages branch May 1, 2020 23:22
github-actions bot pushed a commit that referenced this pull request May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants