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

added support for enums (or other string convertable types) as keys to m... #333

Merged
merged 3 commits into from
Oct 3, 2013
Merged

Conversation

patefacio
Copy link
Contributor

...aps for json serialization

@s-ludwig
Copy link
Member

s-ludwig commented Oct 3, 2013

Thanks, this is a good idea. The only thing that needs to be rebalanced now a bit is serialization vs. deserialization. The idea is that anything that can be serialized must also be deserializable, so a check is needed in serializeJson to see if the key type can also be used as to!Key(string) and only then allow it. Also, handling types that define both toString and fromString (i.e. isStringSerializable!Key) would be nice (I think to!Key(string) doesn't look for a fromString static method).

Should you be willing to extend it prior to pulling, that would be great! Otherwise I can pull it and do the rest of the changes myself (probably early next week).

@patefacio
Copy link
Contributor Author

Great, thanks. Good ideas - I'll certainly give it a go and see if I can extend as you suggest.


From: Sönke Ludwig [email protected]
To: rejectedsoftware/vibe.d [email protected]
Cc: Daniel Davidson [email protected]
Sent: Thursday, October 3, 2013 1:20 AM
Subject: Re: [vibe.d] added support for enums (or other string convertable types) as keys to m... (#333)

Thanks, this is a good idea. The only thing that needs to be rebalanced now a bit is serialization vs. deserialization. The idea is that anything that can be serialized must also be deserializable, so a check is needed in serializeJson to see if the key type can also be used as to!Key(string) and only then allow it. Also, handling types that define both toString and fromString (i.e. isStringSerializable!Key) would be nice (I think to!Key(string) doesn't look for a fromString static method).
Should you be willing to extend it prior to pulling, that would be great! Otherwise I can pull it and do the rest of the changes myself (probably early next week).

Reply to this email directly or view it on GitHub.

improve serialization of AA to serialize if possible, skip otherwise
} else static if(isStringSerializable!(TK)) {
ret[key.toString()] = serializeToJson(value);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is very wrong with whitespaces in this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I don't quite understand it... I think it looks ok in the source:
https://github.com/patefacio/vibe.d/blob/master/source/vibe/data/json.d

}elsestaticif(isAssociativeArray!T){ aliastypeof(T.init.values[0])TV; aliasKeyType!TTK; Unqual!TV[TK]dst; foreach(stringkey,value;src){ staticif(is(TK==string)){ dst[key]=deserializeJson!(Unqual!TV)(value); }elsestaticif(is(TK==enum)){ dst[to!(TK)(key)] = deserializeJson!(Unqual!TV)(value); }elsestaticif(isStringSerializable!TK){ autodsk=TK.fromString(key); dst[dsk]=deserializeJson!(Unqual!TV)(value); } } returndst;


From: Михаил Страшун [email protected]
To: rejectedsoftware/vibe.d [email protected]
Cc: Daniel Davidson [email protected]
Sent: Thursday, October 3, 2013 9:31 AM
Subject: Re: [vibe.d] added support for enums (or other string convertable types) as keys to m... (#333)

In source/vibe/data/json.d:

@@ -899,8 +899,16 @@ Json serializeToJson(T)(T value)
return Json(ret);
} else static if( isAssociativeArray!TU ){
Json[string] ret;

  • foreach( string key, value; value )
    
  •     ret[key] = serializeToJson(value);
    
  •         alias KeyType!T TK;
    
  • foreach( key, value; value ) {
    
  •                     static if(is(TK == string)) {
    
  •                            ret[key] = serializeToJson(value);
    
  •                     } else static if(is(TK == enum)) {
    
  •                            ret[to!string(key)] = serializeToJson(value);
    
  •                     } else static if(isStringSerializable!(TK)) {
    
  •                            ret[key.toString()] = serializeToJson(value);
    
  •                     }
    
  •            } 
    
    Something is very wrong with whitespaces in this block.

    Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

You've used spaces rather than tabs. Don't you use an editor that visualize tabs? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What makes you say that? Do you see spaces in the source?

screen shot 2013-10-03 at 11 09 57 am

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Selecting text in github shows there are spaces in this code, while tabs everywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checked source file as-is and it has tabs with extra trailing single space. Maybe that is what confuses github.

@s-ludwig
Copy link
Member

s-ludwig commented Oct 3, 2013

Looks good as far as I can see. I'll double check the whitespace after merging. The display on GitHub is often strange and I generally completely ignore that and look at the actual code instead. It's a nice opportunity for improvement, though. Is there a public issue tracker for github itself somewhere?

s-ludwig added a commit that referenced this pull request Oct 3, 2013
added support for enums (or other string convertable types) as keys to m...
@s-ludwig s-ludwig merged commit f1c239e into vibe-d:master Oct 3, 2013
@mihails-strasuns
Copy link
Contributor

Is there a public issue tracker for github itself somewhere?

Not I am aware of.

@patefacio
Copy link
Contributor Author

No worries on this though, it was my fault. There were indent issues (I was looking at the wrong place).
Sorry about that - I use emacs but my D mode is quite different so I was going back and forth between D mode and text mode
and doing a slew of tabify calls.

Thanks
Dan


From: Михаил Страшун [email protected]
To: rejectedsoftware/vibe.d [email protected]
Cc: Daniel Davidson [email protected]
Sent: Thursday, October 3, 2013 3:32 PM
Subject: Re: [vibe.d] added support for enums (or other string convertable types) as keys to m... (#333)

Is there a public issue tracker for github itself somewhere?
Not I am aware of.

Reply to this email directly or view it on GitHub.

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

Successfully merging this pull request may close these issues.

4 participants