gizmo icon indicating copy to clipboard operation
gizmo copied to clipboard

ResultHandle getType() has different access level than the ResultType that gets passed back

Open holly-cummins opened this issue 1 year ago • 4 comments

I just tripped over a little inconsistency in ResultHandle. The getResultType method is public:

    public ResultType getResultType() {
        return this.resultType;
    }

... but the ResultType class is package-private.

image

I'd submit a PR to either make the method package private or make the type public, but I wasn't sure which was preferable. Maybe there's a reason for it to be package private?

holly-cummins avatar Jan 04 '24 18:01 holly-cummins

Given that ResultType is an enum (i.e. immutable and thus relatively foolproof), IMO it's probably OK to just make it public.

dmlloyd avatar Jan 04 '24 20:01 dmlloyd

One reason to make the method package private (maybe, sorta) is that getType() is package private, and it's arguably the more useful of the two methods. So it might be strangely inconsistent to have getResultType public but not getType.

holly-cummins avatar Jan 04 '24 20:01 holly-cummins

We could make that public too I think. At least I see no reason why not.

dmlloyd avatar Jan 04 '24 21:01 dmlloyd

I don't think getResultType() is ever useful outside of Gizmo internals. I would make that method package-private, personally.

On the other hand, getType(), which is the "static" type of the value, that indeed can be useful, except the String representation is super user-unfriendly...

Ladicek avatar Jan 24 '24 10:01 Ladicek