Add support for default property values #2100

externl · 2024-05-01T18:42:53Z

This PR adds support for setting default property values in PropertyNames.xml.

I've only added a handful of defaults for now, we can add more in follow-up PRs.

In addition it also adds three new property functions for fetching these properties:

getIceProperty
getIcePropertyAsInt
getIcePropertyAsList

Additionally, JavaScript's setProperty validation was broken. I fixed it for this PR and will open an issue to fix 3.7.

Fixe #2072

- getIceProperty - getIcePropertyAsInt - getIcePropertyAsList

externl · 2024-05-01T18:57:47Z

config/makeprops.py

-            self.srcFile.write(
-                r'    new Property("/^%s\.%s/", false, null),\n'
-                % (self.currentSection, self.fix(propertyName))
+            line = 'new Property("^{section}\\.{name}$", {defaultValue}, {deprecated}, {deprecatedBy})'.format(


The regex for JavaScript was broken. The /XXX/ wrapping is incorrect.

externl · 2024-05-01T18:59:07Z

cpp/src/Ice/Properties.cpp

+    /// @param key The property name.
+    /// @param logWarnings Whether to log relevant warnings.
+    /// @return The property if found, nullopt otherwise.
+    optional<Property> findProperty(string_view key, bool logWarnings)


In the various languages, this lookup logic was only found in setProperty. I've moved this code into a new function.

externl · 2024-05-01T19:02:16Z

scripts/tests/Ice/properties.py



 class PropertiesTestSuite(TestSuite):
    def setup(self, current):
+        if isinstance(self.getMapping(), PhpMapping) or isinstance(


PHP and Matlab don't have any config file tests

cpp/include/Ice/Properties.h

bernardnormier · 2024-05-01T19:18:30Z

cpp/src/Ice/Properties.cpp

+    /// @param key The ice property name.
+    /// @return The default value for the property.
+    /// @throws std::invalid_argument if the property is unknown.
+    string getDefaultValue(string_view key)


Return a string_view since the backing type is a const char*?

cpp/src/Ice/Properties.cpp

bernardnormier · 2024-05-01T19:34:21Z

csharp/src/Ice/PropertiesI.cs

@@ -89,6 +106,12 @@ public string[] getPropertyAsList(string key)
        return getPropertyAsListWithDefault(key, null);
    }

+    public string[] getIcePropertyAsList(string key)
+    {
+        string[] defaultList = IceUtilInternal.StringUtil.splitString(getDefaultProperty(key), ", \t\r\n");


We should use https://learn.microsoft.com/en-us/dotnet/api/system.string.split?view=net-8.0

This is not the same as our splitSting deals with "quoted" properties.

bernardnormier · 2024-05-01T19:36:47Z

csharp/src/Ice/Property.cs

@@ -4,9 +4,10 @@ namespace IceInternal;

 public sealed class Property
 {
-    public Property(string pattern, bool deprecated, string deprecatedBy)
+    public Property(string pattern, string defaultValue, bool deprecated, string deprecatedBy)


What about using a readonly record class?

Let's do that in a follow-up PR.

bernardnormier · 2024-05-01T19:37:13Z

csharp/src/Ice/Property.cs

@@ -17,6 +18,12 @@ public string
        return _pattern;
    }

+    public string
+    defaultValue()


This should be a C# property.

I'll clean up this class in a follow-up pr.

pepone

Looks good

pepone · 2024-05-02T15:13:33Z

cpp/src/Ice/Properties.cpp

+                    // Check for case-insensitive match.
+
+                    if (IceUtilInternal::match(
+                            IceUtilInternal::toUpper(string{key}),


I think we can update toUpper/toLower to work with string_view in a follow up PR, what do you think?

I think we should get rid of this check entirely

cpp/src/Ice/Properties.cpp

pepone · 2024-05-02T15:25:04Z

cpp/src/Ice/PropertyNames.cpp

-// Copyright (c) ZeroC, Inc. All rights reserved.
-//
-// Generated by makeprops.py from file ../config/PropertyNames.xml, Fri Apr 26 11:24:58 2024
+// Copyright (c) ZeroC, Inc. All rights reserved.// Generated by makeprops.py from file ./config/PropertyNames.xml, Wed


Seems there is a missing new line in the generator for this file.

pepone · 2024-05-02T15:26:49Z

cpp/src/Ice/PropertyNames.h

-// Copyright (c) ZeroC, Inc. All rights reserved.
-//
-// Generated by makeprops.py from file ../config/PropertyNames.xml, Fri Apr 26 11:24:58 2024
+// Copyright (c) ZeroC, Inc. All rights reserved.// Generated by makeprops.py from file ./config/PropertyNames.xml, Wed


missing new line in the generator

pepone · 2024-05-02T15:29:39Z

cpp/test/Ice/properties/Client.cpp

+        test(retryIntervals.size() == 1);
+        test(retryIntervals[0] == "0");
+        cout << "ok" << endl;
+    }


To be complete we should also test that you don't get the default when the property is set to a different value. But maybe not worth because the code is simple enough?

I don't think it's needed. The rest of the tests will cover this when we start using it.

pepone · 2024-05-02T15:38:02Z

php/src/Properties.cpp

+    Ice::PropertiesPtr _this = Wrapper<Ice::PropertiesPtr>::value(getThis());
+    assert(_this);
+
+    string propName(name, nameLen);


Can we create a string_view?

Co-authored-by: Jose <[email protected]>

pepone · 2024-05-02T16:39:44Z

cpp/include/Ice/Properties.h

+         * Get an Ice property by key. If the property is not set, its default value is returned.
+         * @param key The property key.
+         * @return The property value or the default value.
+         * @see #setProperty


We should document @throws std::invalid_argument for getIceXxx methods.

pepone · 2024-05-02T16:48:02Z

csharp/src/Ice/PropertiesI.cs

+        // If the property is deprecated by another property, use the new property key
+        if (prop != null && prop.deprecatedBy() != null)
+        {
+            key = prop.deprecatedBy();


This is unrelated to your changes, but the behavior here seems odd. The code assumes the new property values are compatible with the deprecated property.

Yes, I want to get rid of this code as well.

…es (zeroc-ice/ice#2100) Closes zeroc-ice/ice#2072

Add new property functions

2a49349

- getIceProperty - getIcePropertyAsInt - getIcePropertyAsList

externl changed the title ~~Add new property functions~~ Add Ice property functions May 1, 2024

externl changed the title ~~Add Ice property functions~~ Add support for default property values May 1, 2024

externl added 4 commits May 1, 2024 14:46

Restore from main

fbf16f2

.NET format

91abf10

Remove some files

905aa8b

Settings fix

aefbcc4

externl commented May 1, 2024

View reviewed changes

externl requested review from bernardnormier, pepone and InsertCreativityHere May 1, 2024 19:02

externl added 2 commits May 1, 2024 15:04

test, not assert

149dbe5

Fixes

a6a3aed

bernardnormier approved these changes May 1, 2024

View reviewed changes

externl mentioned this pull request May 1, 2024

Ice property validation is broken in JavaScript #2102

Open

Merge branch 'main' into 2072-default-props

71397aa

externl removed the request for review from InsertCreativityHere May 2, 2024 14:56

externl mentioned this pull request May 2, 2024

What about using a readonly record class? #2104

Closed

externl marked this pull request as ready for review May 2, 2024 15:17

Review fixes

821a84d

pepone approved these changes May 2, 2024

View reviewed changes

externl and others added 4 commits May 2, 2024 11:56

Update cpp/src/Ice/Properties.cpp

7657f74

Co-authored-by: Jose <[email protected]>

Update cpp/src/Ice/Properties.cpp

1e4d07b

Co-authored-by: Jose <[email protected]>

Merge remote-tracking branch 'origin/main' into 2072-default-props

6519355

Fix property gen

41bebb1

externl force-pushed the 2072-default-props branch from 5e368df to 41bebb1 Compare May 2, 2024 16:35

externl added 2 commits May 2, 2024 12:36

Use string_view in PHP

b4fe264

Fix MATLAB

ff4c917

pepone approved these changes May 2, 2024

View reviewed changes

externl added 4 commits May 2, 2024 13:33

Fix build

8d4138a

Fix doc comment

1da54f8

Merge branch 'main' into 2072-default-props

d9413d8

.NET format

121f555

This was referenced May 2, 2024

I think we can update toUpper/toLower to work with string_view in a follow up PR, what do you think? #2106

Closed

Remove deprecatedBy from properties #2107

Closed

externl merged commit 6f9de65 into zeroc-ice:main May 2, 2024
17 checks passed

externl deleted the 2072-default-props branch May 2, 2024 18:43

InsertCreativityHere pushed a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 1, 2025

HEAD:(zeroc-ice/ice@6f9de65999) Add support for default property valu…

1fcedd3

…es (zeroc-ice/ice#2100) Closes zeroc-ice/ice#2072

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for default property values #2100

Add support for default property values #2100

externl commented May 1, 2024 •

edited

Loading

externl May 1, 2024 •

edited

Loading

externl May 1, 2024 •

edited

Loading

externl May 1, 2024

bernardnormier May 1, 2024

bernardnormier May 1, 2024

externl May 2, 2024

bernardnormier May 1, 2024

externl May 2, 2024

bernardnormier May 1, 2024

externl May 2, 2024

pepone left a comment

pepone May 2, 2024

externl May 2, 2024

pepone May 2, 2024

pepone May 2, 2024

pepone May 2, 2024

externl May 2, 2024

pepone May 2, 2024

pepone May 2, 2024

pepone May 2, 2024

externl May 2, 2024

Add support for default property values #2100

Add support for default property values #2100

Conversation

externl commented May 1, 2024 • edited Loading

externl May 1, 2024 • edited Loading

Choose a reason for hiding this comment

externl May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl commented May 1, 2024 •

edited

Loading

externl May 1, 2024 •

edited

Loading

externl May 1, 2024 •

edited

Loading