Skip to content

Add legacy constructor to CsvParseSpec and DelimitedParseSpec for backward compatibility#4388

Merged
jon-wei merged 3 commits intoapache:masterfrom
jihoonson:csv-legacy-constructor
Jun 9, 2017
Merged

Add legacy constructor to CsvParseSpec and DelimitedParseSpec for backward compatibility#4388
jon-wei merged 3 commits intoapache:masterfrom
jihoonson:csv-legacy-constructor

Conversation

@jihoonson
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson commented Jun 8, 2017

Some external users are using this API, so the legacy constructor is needed (#4254 (comment)).


This change is Reviewable


@Deprecated
public CSVParseSpec(
@JsonProperty("timestampSpec") TimestampSpec timestampSpec,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think you need the @JsonProperty annotations here since only the main constructor is @JsonCreator, this legacy constructor would only be used when directly instantiating

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're correct. Removed them.

@leventov
Copy link
Copy Markdown
Member

leventov commented Jun 9, 2017

If so maybe also need to add constructors for other classes altered in #4254, e. g. DelimitedParseSpec

@jihoonson
Copy link
Copy Markdown
Contributor Author

@leventov thanks. I added it too. CSVFlatDataParser and TSVFlatDataParser in extensions-core are changed as well. Should they also need to be backward compatible?

@leventov
Copy link
Copy Markdown
Member

leventov commented Jun 9, 2017

@jihoonson IMO not needed

@jihoonson jihoonson changed the title Add legacy constructor to CsvParseSpec for backward compatibility Add legacy constructor to CsvParseSpec and DelimitedParseSpec for backward compatibility Jun 9, 2017
@jon-wei jon-wei merged commit bbc307b into apache:master Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants