[Scummvm-devel] [scummvm] ENGINES: Add a thorough explanation of the ugliness caused by _singleId (65c60ef)

Filippos Karapetis md5 at scummvm.org
Tue Feb 28 23:25:33 CET 2012


fuzzie has covered what I was about to say already, but I'll have to add my
2c here.

In my eyes, this is an undocumented (and annoying) feature that is there
just to cancel another feature (game IDs). I wrote the comment this way
because this code bypasses a whole feature on purpose, without even
specifying what it does... I was pulling my hair trying to figure out why
kyra is returning a game ID in the advanced detector but SCI isn't (which
is bad, common code should be documented). The fact that this code has been
around for long does not mean it works, it is a workaround (at least,
silently changing a feature has been considered a workaround historically).

I did not change any functionality in the common code, I just added a
comment about why this functionality offered by singleId is not ideal, at
least not IMHO, since it just changes a default behavior and interferes
with other common code. And worst of all, this wasn't documented anywhere,
hence my commit and subsequent revert.

As for a discussion: this was code that was just committed in the past, and
I don't recall why it was added and if there was a discussion concerning it
in the first place (at least I couldn't find any...).

Regards
Filippos

On Tue, Feb 28, 2012 at 11:17 PM, Alyssa Milburn <
reply+c-1023204-5c06e562b6391148f90d33579bf4201a13427d98-510395 at reply.github.com
> wrote:

> Silently replacing the detected gameid with the singleid is an ugly
> workaround which is completely surprising in behaviour and was still
> completely undocumented. Searching for 'singleid' on the mailing list
> doesn't produce any results. If one of you has better documentation, please
> add it.
>
> I expressed surprise at [md5]'s SCI commit earlier, which was obviously in
> reaction to discovery that the detector was overwriting the gameid
> information with this weird 'singleid' thing, and [md5] came on IRC and
> agreed to revert the commit until a better solution was found for
> identifying the games which configuration entries correspond to without
> having to re-run the detector.
>
> I explained that I thought some of the aim of this was to avoid polluting
> the global namespace, and so simply removing singleid was a bad idea. If
> that isn't the case, and we can simply remove singleid from engines as we
> wish, then obviously that simplifies things. But I don't think it's fair to
> act surprised at confusion over this completely undocumented and annoying
> "feature".
>
> ---
> Reply to this email directly or view it on GitHub:
>
> https://github.com/scummvm/scummvm/commit/65c60ef02737f9e6a7e4091b622a6a1e0871d6d7#commitcomment-1023204
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20120229/e2cf20c1/attachment.html>


More information about the Scummvm-devel mailing list