refactor(Engine): nits from context work review

[emyller]

Contributes to #111.

Context: #109 (comment)

[emyller]

1. In src/Utils/Mappers.php, consider removing feature ID from overridesKey key.

Attempted in bd26178. @gagantrivedi What's the benefit you see from this? Or please let me know if I didn't follow correctly.

2. Consider renaming flagsmith_id segment and feature metadata fields to id.

Done in 31280ca. However, the rationale that supports flagsmith_id is based on the engine being, at least to an inspirational level, agnostic to the vendor. So metadata isn't assumed to be Flagsmith metadata, hence flagsmith_id was chosen by the Flagsmith client — I believe a bit of namespacing looks nicer indeed. @gagantrivedi Unless you have a good argument here (I'm very open), I'd like to revert this commit, and suggest some team thought towards making it consistent across SDKs. cc @khvn26 @Zaimwa9 Please weigh in if you'd like.

[gagantrivedi]

1. In src/Utils/Mappers.php, consider removing feature ID from overridesKey key.

Attempted in bd26178. @gagantrivedi What's the benefit you see from this? Or please let me know if I didn't follow correctly.

Sorry, I don’t see much value in this anymore. I think my point was that it’s used to generate a unique combination, but a unique combination can be generated without the feature ID. To sum it all up, feel free to ignore this comment.

[gagantrivedi]

Done in 31280ca. However, the rationale that supports flagsmith_id is based on the engine being, at least to an inspirational level, agnostic to the vendor. So metadata isn't assumed to be Flagsmith metadata, hence flagsmith_id was chosen by the Flagsmith client — I believe a bit of namespacing looks nicer indeed. @gagantrivedi Unless you have a good argument here (I'm very open), I'd like to revert this commit, and suggest some team thought towards making it consistent across SDKs. cc @khvn26 @Zaimwa9 Please weigh in if you'd like.

My only concern is that this added some cognitive overhead during the review—I had to look up its origin and usage to understand it's just the feature ID

[emyller]

Sorry, I don’t see much value in this anymore. I think my point was that it’s used to generate a unique combination, but a unique combination can be generated without the feature ID. To sum it all up, feel free to ignore this comment.

I agree there's no much value in the code change per se, but I do think it's still a valid point to make, especially because it makes it clearer that feature names are indeed unique (per project). I've seen several places in the code where this isn't assumed (sorry I don't have any examples ready), perhaps because it's not clear anywhere in the data model. Therefore, I vouch to keep this change.

[emyller]

My only concern is that this added some cognitive overhead during the review—I had to look up its origin and usage to understand it's just the feature ID

I see. That's a good point. I think this is yet another consequence of our own rushing through this SDK work without a clear roadmap. I also think we should agree on the field naming, for the sake of consistency across code bases and team alignment.

Given my argument above, which is admittedly based on my own assumption, I'd vote towards flagsmith_id.

[emyller]

I also think we should agree on the field naming, for the sake of consistency across code bases and team alignment.

We soft-decided on id, based on a vote between the available engineers involved with this topic. In that case, this patch is ready to go.

[khvn26]

LGTM