Add ETD CSV parser #1556
Add ETD CSV parser #1556
Conversation
Extensible parser using a plugin Update trade parser to use plugin and parse security trades
Add `isKnownFormat()` method
jodastephen
force-pushed
the
topic/etd-csv
branch
from
35aecaf
to
f1a346f
Compare
| */ | ||
| public static int parseInteger(String str) { | ||
| return Integer.parseInt(str); | ||
| } |
There was a problem hiding this comment.
Is this method adding any value? Calling Integer.parseInt() directly seems good enough
There was a problem hiding this comment.
I'm assuming that we may add additional error handling or format handling later. With doubles in other projects we have supported (20) to mean -20.
| * <p> | ||
| * The following standard columns are supported:<br /> | ||
| * <ul> | ||
| * <li>The 'Strata Position Type' column is option, and defines the instrument type, |
| * If that column is not found, the 'Long Quantity' and 'Short Quantity' columns will be used instead. | ||
| * <p> | ||
| * The single 'Expiry' column will normally just be set. | ||
| * Flex futures will also set the 'Expiry Day', 'Settlement Type' and 'Exercise Style'. |
There was a problem hiding this comment.
Should this comment reference options rather than futures?
| ValueWithFailures<List<T>> result = ValueWithFailures.of(ImmutableList.of()); | ||
| for (CharSource charSource : charSources) { | ||
| ValueWithFailures<List<T>> singleResult = parseFile(charSource, positionType); | ||
| result = result.combinedWith(singleResult, Guavate::concatToList); |
There was a problem hiding this comment.
Static import concatToList() for readibility
There was a problem hiding this comment.
Its a method reference 😄
| // handle longQuantity and shortQuantity | ||
| double longQuantity = parseQuantity(row, LONG_QUANTITY_FIELD); | ||
| double shortQuantity = parseQuantity(row, SHORT_QUANTITY_FIELD); | ||
| double quantity = longQuantity - shortQuantity; |
There was a problem hiding this comment.
Probably best to throw an exception or create a failure if quantity, short quantity and long quantity are all not populated.
Silently setting quantity to 0 feels dangerous; it's reasonable to expect clients to always provide a quantity (can always be manually set to 0 in the input if needed)
| private static EtdSettlementType parseEtdSettlementType(String str) { | ||
| String upper = str.toUpperCase(Locale.ENGLISH); | ||
| Map<String, EtdSettlementType> valuesByCode = | ||
| Stream.of(EtdSettlementType.values()).collect(toImmutableMap(EtdSettlementType::getCode)); |
There was a problem hiding this comment.
Could store a static version of this map rather than recreating it every time the method is called.
| * @param builder the builder to update | ||
| */ | ||
| public default void parsePositionAttributes(CsvRow row, PositionInfoBuilder builder) { | ||
| // do nothing |
There was a problem hiding this comment.
Given that these are builder mutating methods, 'populate' might be a better name than 'parse'.
Are we assuming that overrides of these methods may set non-attribute values on the builder (e.g. settlement date, trade date)?
If yes then parseInfo would be a better name than parseAttributes
If the methods are only expected to populate attributes then we could return a map of attributes and allow the clients to populate the builder
| public void test_parseInteger() { | ||
| assertEquals(LoaderUtils.parseInteger("2"), 2); | ||
| assertThrowsIllegalArg(() -> LoaderUtils.parseInteger("Rubbish")); | ||
| } |
There was a problem hiding this comment.
This is just a test of core java functionality
jodastephen
force-pushed
the
topic/etd-csv
branch
from
97bb6f7
to
f1c4f87
Compare
Add
PositionCsvLoaderto complementTradeCsvLoader.Update trade loader to parse security trades.
Add extensibility to parser using a plugin, to pickup attributes.