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

Improve CreateNative failure message #903

Merged
merged 3 commits into from Oct 10, 2018
Merged

Improve CreateNative failure message #903

merged 3 commits into from Oct 10, 2018

Conversation

@asherkin
Copy link
Member

@asherkin asherkin commented Oct 9, 2018

This confuses everyone.

This confuses everyone.
@asherkin asherkin requested review from Drifter321 and Headline Oct 9, 2018
@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented Oct 9, 2018

Docs are definitely an improvement. Do we care that we call these Fake Natives internally; but Dynamic Natives to pawn? Anyone digging through code will know them by the former name.

@asherkin
Copy link
Member Author

@asherkin asherkin commented Oct 9, 2018

I belive the intention is that they become just "natives" in the future, and the term "fake" has confused people in the past, I figured "dynamic" was clear but happy to change/remove. I think dropping the qualifier would make the most sense if anything.

@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented Oct 9, 2018

Yeah; if Fake/Dynamic are ephemeral ("") qualifiers let's drop them entirely to userspace.

@peace-maker
Copy link
Member

@peace-maker peace-maker commented Oct 9, 2018

You could even go as far as adding the name of the plugin, that has the native already registered in the second error.

Copy link
Member

@Headline Headline left a comment

Good improvement, cool with taking as-is, but I also like Peace-Maker's suggestion at identifying the other plugin's name so debugging is easier so if you want to go the extra mile then go for it, otherwise no problem

@asherkin asherkin merged commit 7dd733c into master Oct 10, 2018
2 checks passed
@Headline Headline deleted the asherkin-patch-1 branch Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants