Skip to content

panc: only allow digit-only string as candidate list index (and allow anything else as dict key)#157

Merged
ned21 merged 1 commit intoquattor:masterfrom
stdweird:dict_keys_digits
Nov 3, 2017
Merged

panc: only allow digit-only string as candidate list index (and allow anything else as dict key)#157
ned21 merged 1 commit intoquattor:masterfrom
stdweird:dict_keys_digits

Conversation

@stdweird
Copy link
Member

@stdweird stdweird commented Jun 23, 2017

Fixes #156
This is backwards incompatible because eg hex and/or octal notations cannot be used anymore.

@stdweird stdweird changed the title panc: only allow digit-only as candidate list indices (and allow anything else as dict key) panc: only allow digit-only string as candidate list index (and allow anything else as dict key) Jun 23, 2017
@stdweird stdweird force-pushed the dict_keys_digits branch 2 times, most recently from 684704b to a97075a Compare June 23, 2017 17:22
@stdweird
Copy link
Member Author

We can introduce hex if it is really used, and/or also get rid of octal support (which is confusing imho)

@gombasg
Copy link
Contributor

gombasg commented Jul 26, 2017

I don't care much about octal/hex notion (I don't remember ever using it), but I have a feeling that relaxing the rules for dict keys would break XML output. The original rules came from the requirements for XML element and attribute names. Automatic escaping would be possible, but that would require changes to the CCM library, and would open the can of worms about how to detect if something is escaped.

@stdweird
Copy link
Member Author

@gombasg can you give an example of what would render invalid xml?

we should have another discussion at the next workshop on escaping. i tought we could get rid of it altogether (some encoding/decoding might be required; but not the escaping we have now).
@Piojo had some idea about this

@jouvin
Copy link
Contributor

jouvin commented Oct 26, 2017

About the escaping discussion, see very old issues: #17, #32, #35

@ned21
Copy link
Contributor

ned21 commented Oct 26, 2017

Discussion at RAL workshop: OK in principle. @stdweird will additionally drop octal/hex. Then it just needs to be code reviewed & can be merged.

Once merged and released then Aquilon can be updated to address: quattor/aquilon#54

@ned21 ned21 added this to the 10.5 milestone Oct 26, 2017
@stdweird
Copy link
Member Author

@jrha @ned21 @jouvin @gombasg after dropping octal mode, what to do with leading 0?
eg /a/09 is same path as /a/9? or is 09 a syntax error?

@jrha
Copy link
Member

jrha commented Oct 27, 2017

We're removing support for octal, so my vote would be for it to be syntax error rather than silently accepting the value with a different meaning to previous compiler versions.

@stdweird
Copy link
Member Author

@jrha we do that for hex. it is now seen as a key in a dict. btw we can do that also for octals: digits with leading 0 can be used as keys in a dict, so /adict/042 would be ok.
@ned21 would that help with your issue in aquilon? would have to rename the room (or what was it again) to 042.

@ned21
Copy link
Contributor

ned21 commented Oct 27, 2017

@stdweird There is no existing problem with aquilon, it only accepts what the PAN compilers accepts. The specific request is for Aquilon to support rooms named 206-1-001.

@stdweird
Copy link
Member Author

@ned21 ok. but what is your opinion on the octal numbers: syntax error or valid dict key?

@ned21
Copy link
Contributor

ned21 commented Oct 28, 2017

Leading zeros are ambiguous (is 09 == 9?) so I agree with @jrha we should make them a syntax error.

@stdweird
Copy link
Member Author

stdweird commented Nov 2, 2017

@ned21 @jrha i removed octal support. this is blocking #170 and #171

Copy link
Member

@jrha jrha left a comment

Choose a reason for hiding this comment

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

LGTM!

@ned21 ned21 merged commit 4adb705 into quattor:master Nov 3, 2017
@jrha jrha modified the milestones: 10.6, 10.5 Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants