-
Notifications
You must be signed in to change notification settings - Fork 26
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
make uk.ac.starlink.table.BeanStarTable recognise Date #17
Comments
What do you mean by recognise? I think that BeanStarTable treats Date properties like those of any other (non-primitive) class, including String, which is to expose them as table columns. |
I have a an object that does not have any fancy BeanInfo, but does have getters and setters with dates which are not appearing. On a quick look at the code https://github.com/Starlink/starjava/blob/master/table/src/main/uk/ac/starlink/table/BeanStarTable.java#L224 it seemed that only primitives and Strings were being allowed. |
Oh yes, my mistake, sorry Paul. Actually I'm somewhat relieved. There's nothing in the StarTable interface itself that restricts column content classes, but much of the surrounding infrastructure (e.g. de/serializers) will not work very well in the presence of columns with types that are not either primitive or String. So I don't want to actively encourage use of other types in StarTables. In any case, I don't plan to provide Date-class or time-representation in general throughough the STIL framework, which would be a big job and hard to get right. But if you want to have a BeanStarTable that exposes bean properties of type Date or other non-STIL-friendly classes it should probably be possible. How about if I adjust the BeanStarTable API a little bit so that you can configure which classes are recognised, e.g. replace the existing
|
On 2017-04 -06, at 11:07, Mark Taylor ***@***.***> wrote:
Actually I'm somewhat relieved. There's nothing in the StarTable interface itself that restricts column content classes, but much of the surrounding infrastructure (e.g. de/serializers) will not work very well in the presence of columns with types that are not either primitive or String. So I don't want to actively encourage use of other types in StarTables.
In any case, I don't plan to provide Date-class or time-representation in general throughough the STIL framework, which would be a big job and hard to get right.
But if you want to have a BeanStarTable that exposes bean properties of type Date or other non-STIL-friendly classes it should probably be possible. How about if I adjust the BeanStarTable API a little bit so that you can configure which classes are recognised, e.g. replace the existing private static useProperty method with something like
private boolean useProperty( PropertyDescriptor prop ) {
return prop.getReadMethod() != null
&& ! prop.isHidden()
&& isAcceptableClass( prop.getPropertyType() );
}
protected boolean isAcceptableClass( Class clazz ) {
return pclazz == String.class || pclazz.isPrimitive();
}
That looks good to me - I can then at least do some experiments to see if Dates will make it through to some of the other plotting layers….
You have implemented at least some magic in the de/serializers for ISO formatted Date strings nowadays - though just having had a look at Topcat, I see that the columns do have String class, with “units”, “domain”, or “UCD” presumably giving the automatic “time” behaviour to the column - so perhaps it is not that easy for me to achieve what I would like without also being able to override getObjectType().
The alternative is for me just to do a copy and paste reimplementation of BeanStarTable myself, with my own bit of Date magic in, rather than work out what the ideal extensible BeanStarTable is I guess...
|
I've just read my own javadocs and realised that the suggestion I made above is not so straightforward. The existing private static In most cases, I think where I have treated |
now that time plots are part of stil it would be nice if uk.ac.starlink.table.BeanStarTable could recognise Dates too
The text was updated successfully, but these errors were encountered: