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

Disable 32-bit plugins #1562

Merged
merged 5 commits into from
Aug 16, 2013
Merged

Disable 32-bit plugins #1562

merged 5 commits into from
Aug 16, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Aug 9, 2013

OK, then. One last change before the next release?

I looked at catching the exception, but it turns out for 32-bit plug-ins, it's a very specific exception and we were the ones throwing it, so I just made the changes there.

I initially thought we still needed to throw the exception to prevent the caller from doing …something? But I can't see any reason for it, so I've commented it out.

@skurfer
Copy link
Member Author

skurfer commented Aug 9, 2013

Went ahead and bumped the version, too.

@pjrobertson
Copy link
Member

P.S. hehe - I like how you used 'PlugIns' for the folder name. Still sticking to that convention? ;-)
It goes with the existing folder name, so I have no problems, just thought it was funny

@skurfer
Copy link
Member Author

skurfer commented Aug 12, 2013

It goes with the existing folder name, so I have no problems, just thought it was funny

Yeah, I'm not a fan, but I thought I'd keep it consistent. I think I'll change "32-bit" to "disabled" so we don't need to create a new folder for every situation where this happens. (And who knows. Someone could have some PPC-only plug-ins out there.)

@skurfer
Copy link
Member Author

skurfer commented Aug 12, 2013

I won't merge, since I didn't exactly do what you suggested. I'll let you look it over.

@pjrobertson
Copy link
Member

Still could be a little more descriptive - as to 'why' it's unsupported (I remember we got lots of users in the past saying "my plugin says 'Architecture unsupported', why doesn't it work" - even that was too 'generic' for people to understand what it meant ;-)

Everything else looks good.

@skurfer
Copy link
Member Author

skurfer commented Aug 12, 2013

Well, I was trying to make it generic to future-proof it. There's only one reason for it to happen at this point, but do we want to hard-code the message for that? I will if you really think it’s a good idea.

@pjrobertson
Copy link
Member

I see your point: isSupported could mean anything in the future. Hmmm... I think it would be good to be a bit more descriptive. If we ever have more than one reason we can change the message to be generic then. Start specific then generalise. That's the way things are normally done right? :P

@skurfer
Copy link
Member Author

skurfer commented Aug 12, 2013

How’s that for descriptive? I also caught myself writing it correctly as “plug-in” last time around. I’ve “fixed” that too. 😉

@pjrobertson
Copy link
Member

I'd prefer a 400 word essay, but that'll do. Cheers for that :)

@skurfer
Copy link
Member Author

skurfer commented Aug 16, 2013

Any problems with this? I think it's all we're waiting on for the next pre-release (unless you want to do transifex, too).

@pjrobertson
Copy link
Member

Oh yeah, sorry.

I'll do transifex in a bit. Good idea.

pjrobertson added a commit that referenced this pull request Aug 16, 2013
@pjrobertson pjrobertson merged commit dbf3c07 into release Aug 16, 2013
@pjrobertson pjrobertson deleted the disablePlugins branch August 16, 2013 14:28
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.

None yet

2 participants