Skip to content
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

Inconsistency with numeric keys in attributes map #604

Closed
jmini opened this issue Dec 20, 2017 · 4 comments
Closed

Inconsistency with numeric keys in attributes map #604

jmini opened this issue Dec 20, 2017 · 4 comments
Milestone

Comments

@jmini
Copy link
Contributor

jmini commented Dec 20, 2017

Take this simple example:

String s = "" +
"[#idname.rolename]\n" +
"Lorem ipsum dolor\n";

org.asciidoctor.Asciidoctor asciidoctor = org.asciidoctor.Asciidoctor.Factory.create();
org.asciidoctor.ast.Document document = asciidoctor.load(s, new java.util.HashMap<String, Object>());

Block block = (Block) document.getBlocks().get(0);
Map<String, Object> attributes = block.getAttributes();
System.out.println(attributes);
System.out.println(attributes.keySet().contains("1"));
System.out.println(attributes.containsKey("1"));
System.out.println(attributes.get("1"));

With Version 1.6.0-alpha.6 console output is:

{1=#idname.rolename, role=rolename, id=idname}
true
false
null

I do not expect attributes.keySet().contains("1") and attributes.containsKey("1") to return something different.
Of course attributes.get("1") returns null and this is not what I expected…


I did also some tests with Version 1.5.6 (this imply a other version of JRuby)

Block block = (Block) document.getBlocks().get(0);
Map<String, Object> attributes = block.getAttributes();
System.out.println(attributes);
System.out.println(attributes.keySet().contains("1"));
System.out.println(attributes.containsKey("1"));
System.out.println(attributes.get("1"));

Produces this output:

{1=>"#idname.rolename", "id"=>"idname", "role"=>"rolename"}
false
false
null

Interesting fact, this code snippet:

Map<?, ?> attributes2 = (Map<?, ?>) block.getAttributes();
System.out.println(attributes2);
System.out.println(attributes2.keySet().contains(1));
System.out.println(attributes2.containsKey(1));
System.out.println(attributes2.get(1));

Produces:

{1=>"#idname.rolename", "id"=>"idname", "role"=>"rolename"}
true
true
#idname.rolename

This is a little bit better. But this demonstrates that using String for the key type might not be a god idea.


I tried my second snippet with 1.6.0-alpha.6 and I got:

{1=#idname.rolename, role=rolename, id=idname}
false
false
false

This is really wired…
What do you think?

@robertpanzer
Copy link
Member

I remember that I did some changes for numeric attributes to fix problems when iterating over the attributes and suddenly finding a Long instead of a String in a Map<String,Object>.
IIRC these should not be used by users, but are usually used as positional attributes for extensions.
https://github.com/asciidoctor/asciidoctorj/blob/asciidoctorj-1.6.0/asciidoctorj-core/src/main/java/org/asciidoctor/extension/PositionalAttributes.java

I'll dig into this again once I find the time. The Javadoc of the source above is also crappy, I'll fix this.

@jmini
Copy link
Contributor Author

jmini commented Dec 22, 2017

Thank you for the quick feedback.

Of course we can decide to not expose the numeric keys in the Java Map, because I agree that they are really useful (I do not think that somebody will use the positional attribute in the AST classes, when he has much more convenient methods or attribute to access the same information).

What is really disturbing at the moment is this "the information is here, depending on how you access it". I did not check the Javadoc in details, but I guess that the current state is a violation of the Map interface contract.

robertpanzer added a commit to robertpanzer/asciidoctorj that referenced this issue Dec 29, 2017
robertpanzer added a commit that referenced this issue Jan 7, 2018
Fixes #604. Implement containsKey() and keySet().contains() consisten…
@jmini
Copy link
Contributor Author

jmini commented Jan 11, 2018

Ï tested it with the latest 1.6.0-SNAPSHOT version (from repo https://oss.jfrog.org/artifactory/oss-snapshot-local).

It looks good to me.

@robertpanzer robertpanzer added this to the 1.6.0-alpha.7 milestone Jan 13, 2018
@robertpanzer
Copy link
Member

Fixed with #609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants