Skip to content

Parser interface docs; deprecate setFieldNames/getFieldNames. #35

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.

Sign up for GitHub

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

Merged
drcrallen merged 1 commit into from
Nov 9, 2015
Merged

Parser interface docs; deprecate setFieldNames/getFieldNames. #35

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/main/java/com/metamx/common/parsers/Parser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,30 @@
import java.util.Map;
import java.util.Set;

/**
* Class that can parse Strings into Maps.
*/
public interface Parser<K, V>
{
public Map<K, V> parse(String input) ;
public void setFieldNames(Iterable<String> fieldNames) ;
/**
* Parse a String into a Map.
*
* @throws ParseException if the String cannot be parsed
*/
public Map<K, V> parse(String input);

/**
* Set the fieldNames that you expect to see in parsed Maps. Deprecated; Parsers should not, in general, be
* expected to know what fields they will return. Some individual types of parsers do need to know (like a TSV
* parser) and those parsers have their own way of setting field names.
*/
@Deprecated
public void setFieldNames(Iterable<String> fieldNames);

/**
* Returns the fieldNames that we expect to see in parsed Maps, if known, or null otherwise. Deprecated; Parsers
* should not, in general, be expected to know what fields they will return.
*/
@Deprecated
public List<String> getFieldNames();
Copy link
Copy Markdown
Contributor

@himanshug himanshug Nov 7, 2015

Choose a reason for hiding this comment

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

actually, thinking more about it, it seems support for fieldNames is useful for csv, tsv and the regex, javascript parsers in #36 unlike the JSONParser and they shouldn't be deprecated.

Copy link
Copy Markdown
Contributor Author

@gianm gianm Nov 7, 2015

Choose a reason for hiding this comment

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

@himanshug the thought behind deprecating these was:

  • external usages of setFieldNames are generally immediately after construction of a specific parser class. So those usages don't really need to use a setter- they could instead use a constructor that offers fieldNames (those constructors already exist but are not always used).
  • these methods are actually pretty nonsensical for the JsonPath parser (Add JsonPath parser #34), because its FieldSpecs serve the same purpose, but they do it in a richer way that is specific to JsonPath.

So, I don't think these methods are really useful, nor do they generalize well to all parsers.

Copy link
Copy Markdown
Contributor

@himanshug himanshug Nov 8, 2015

Choose a reason for hiding this comment

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

ok, sounds reasonable, I agree field names do not make sense for json parsers but only for csv, tsv, regex parsers who can support those via specific constructors.

}