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

Remove Support for the tag Keyword #2177

Merged

Conversation

InsertCreativityHere
Copy link
Member

In the "Ice 4.0" days, we replaced the optional keyword to tag, but kept optional for backwards compatibility.
We kept separate tokens for them so we could emit deprecation warnings for optional. Different behavior == different tokens.

Now that "Ice 3.8" is the target, I see no point in supporting tag, so this PR deletes it.
This keyword doesn't exist in 3.7, and there's no reason for anyone to switch from optional to tag.

There isn't much to review just wanted to make sure everyone was aware of & okay with this.


Note that we already removed the deprecation warning. Or maybe it never got backported in the first place.
Without it there is no difference in behavior between the tokens. The code is literally copy/paste duplicates.

@InsertCreativityHere InsertCreativityHere changed the title Remove Support for the tag keyword. Remove Support for the tag Keyword May 14, 2024
@externl
Copy link
Member

externl commented May 14, 2024

I thought our idea was to use the word tag going forward to limit confusion with "optional" and that tag would be prefered. Pretty sure we're using "tag" in various Input/Output streams and in some Slice.

Is there an issue/discussion for removing this?

@InsertCreativityHere
Copy link
Member Author

I thought our idea was to use the word tag going forward to limit confusion

That's going to be a tall order. Updating all our code/docs to use tag.
Especially because we already use the word tag to refer to the number you assign an optional field. As in:
"You can mark a field as optional, and assign it a tag number with: optional(tagNum)"

Pretty sure we're using "tag" in various Input/Output streams and in some Slice.

We're only using tag in one place. A function in Router.ice.

Is there an issue/discussion for removing this?

No, I can make a proper issue if that's preferable to discussing it here on this PR.
Me and Bernard were just discussing it earlier, but indeed, only in person.

@externl
Copy link
Member

externl commented May 14, 2024

I'm fine to remove it I guess but I'm pretty sure we've been sprinkling the word tag all around the code since we started on 3.8 and added tag as an alias for optional.

https://github.com/zeroc-ice/ice/blob/main/cpp/include/Ice/StreamHelpers.h#L396

I'm sure there are more.

This commit introduced several that should be undone. 885ad85

@InsertCreativityHere
Copy link
Member Author

Yes, you're right. I'll look around the code for any references to tag.
And remove those too, if everyone is okay with this going forward in the first place.

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@InsertCreativityHere
Copy link
Member Author

We had a separate issue open for removing the 'tagged's that snuck in: #1623
I'm posting it here to link the two, and just to quote from it: however, the standard term remains 'optional'

@InsertCreativityHere
Copy link
Member Author

I altered this PR to also fix any mentions of the tagged keyword from icerpc.
I think it's pretty trivial, so I'm not remarking reviewers, but feel free to re-re-view if you want!!

@InsertCreativityHere InsertCreativityHere merged commit 0bd9677 into zeroc-ice:main May 20, 2024
16 checks passed
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

4 participants