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

small hack that the underlying problem is visible to the user then the error is displayed with [NSApp presentError:] #658

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schriftgestalt
Copy link
Contributor

No description provided.

…e error is displayed with [NSApp presentError:]
@tiennou
Copy link
Contributor

tiennou commented Oct 27, 2018

It looks like something that should be handled at your level, since Objective-Git doesn't really expect its errors to be passed to presentError: (like, we're completely wrong on how error descriptions are used. This is supposed to be the alert title, so having a format-friendly version is misleading to writers and will cause strange alerts with long strings if used as-is). We also AFAIR never suggest recovery suggestions (they're not in use), only a description and failure reason (and even this is a stretch). Lastly, your addition (though it's not likely to happen) will crash while inserting a nil in a dictionary.

To be clear, I realize our error-building could be enhanced, but this feels too much like a band-aid. So, I'm 👎 on that change, though I'd still like to praise your contribution spirit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants