You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Jonathan Ellis (JIRA)" <ji...@apache.org> on 2009/02/11 19:56:59 UTC
[jira] Created: (THRIFT-339) functions with default values broken
by #242 (r734536)
functions with default values broken by #242 (r734536)
------------------------------------------------------
Key: THRIFT-339
URL: https://issues.apache.org/jira/browse/THRIFT-339
Project: Thrift
Issue Type: Bug
Components: Compiler (Python)
Reporter: Jonathan Ellis
Esteve's last change to how default values are stored broke stuff. Here is a quick example:
{{
service Test
{
bool get_slice(i32 start = -1),
}
}}
generates
{{
class get_slice_args:
thrift_spec = None
def __init__(self, start=thrift_spec[-1][4],):
self.start = start
}}
which is obviously invalid.
I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) the compiler should abort (or warn)
if an argument doesn't have a field key
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672783#action_12672783 ]
Jonathan Ellis commented on THRIFT-339:
---------------------------------------
i thought the field keys are supposed to be optional, which is good, because they fuglify things up quite well. :)
> the compiler should abort (or warn) if an argument doesn't have a field key
> ---------------------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12680432#action_12680432 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
David: great! However, I think the thrift_limits variable is necessary because, as you pointed out, if we have a structure like the one you added, a field with id 3 would be treated as -2. So, if we had a newer version of that structure, which adds another field (with id 3), wouldn't it clash with the old version?
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12673080#action_12673080 ]
David Reiss commented on THRIFT-339:
------------------------------------
function result structures use field 0, so how about a new attribute?
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Kevin Clark (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Clark updated THRIFT-339:
-------------------------------
Fix Version/s: 0.1
Shooting for 0.1, if we get review.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Fix For: 0.1
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672803#action_12672803 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
It's quite late here, so I'm probably asking something stupid, but why is thrift_spec a tuple? Using a dict keyed on field keys, would allow the fastbinary extension to support negative field keys, IMHO. But I'm sure there must be a reason why it isn't, apart from dictionaries being mutable. I wish we could use named tuples in Python 2.4 and 2.5
THRIFT-105 is slightly related to this, so Alexander can explain the problem with negative field keys much better than I.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696727#action_12696727 ]
Jonathan Ellis commented on THRIFT-339:
---------------------------------------
My case does not deal with negative field ids. (In the example I gave, the field _default_ is -1, not the field id.)
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.2
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12680832#action_12680832 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
Using the aforementioned structure (Foo), if I want l (an optional member) to be None so that it's not serialized, it would still end up passing [1,2,3,4] to the underlying transport.
Actually, I don't know what's wrong with the current behavior. The problem is in how thrift_spec for negative field keys is being generated.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) the compiler should abort (or warn)
if an argument doesn't have a field key
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672789#action_12672789 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
I spoke too early, it seems my patch DID actually break things :-) Here's the code that an old compiler generates:
thrift_spec = None
def __init__(self, d=None):
self.start = -1
if isinstance(d, dict):
if 'start' in d:
self.start = d['start']
so, unless we always generate thrift_spec or we use a sentinel, I don't know if this can be fixed. However, I wonder how the fastbinary extension could work if thrift_spec is None. I can change the compiler to generate the thrift_spec variable no matter what, but would like to hear the opinion of others.
> the compiler should abort (or warn) if an argument doesn't have a field key
> ---------------------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Attachment: thrift-339.patch
This patch should fix this issue, I tested it using both the Python-only binary protocol and the native extension. This should fix THRIFT-105 as well.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12673010#action_12673010 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
I implemented it the way you suggested David, the changes to fastbinary.c were minimal and the compiler got a bit simpler. I've tested it, but it needs a review ;-)
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12673060#action_12673060 ]
David Reiss commented on THRIFT-339:
------------------------------------
This looks awesome. There is one thing that I'm a little uneasy about, though, which is that (in my example), a field with id 3 (which should be skipped) would be treated as -2 instead. I think the only solution for this is to provide some extra info to the extension, specifically the maximum expected positive field id, and use that to skip over anything larger.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-339) functions with default
values broken by #242 (r734536)
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672781#action_12672781 ]
esteve edited comment on THRIFT-339 at 2/11/09 1:19 PM:
------------------------------------------------------------------
Although I'm happy to admit any breakage I cause :-) I don't think this issue is valid (or at least, not for the reasons exposed). Note that you're not using a field key, and thus the thrift_spec variable is not populated at all. You can fix this using a positive field key:
service Test {
bool get_slice(1:i32 start = -1),
}
which should generate this code:
class get_slice_args:
thrift_spec = (
None, # 0
(1, TType.I32, 'start', None, -1, ), # 1
)
I think the compiler should abort if it doesn't find a valid field key and don't generate any code, though.
was (Author: esteve):
Although I'm happy to admit any breakage cause :-) I don't think this issue is valid (or at least, not for the reasons exposed). Note that you're not using a field key, and thus the thrift_spec variable is not populated at all. You can fix this using a positive field key:
service Test {
bool get_slice(1:i32 start = -1),
}
I think the compiler should abort if it doesn't find a valid field key and don't generate any code, though.
> functions with default values broken by #242 (r734536)
> ------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Priority: Blocker
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) the compiler should abort (or warn) if
an argument doesn't have a field key
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Priority: Major (was: Blocker)
Summary: the compiler should abort (or warn) if an argument doesn't have a field key (was: functions with default values broken by #242 (r734536))
> the compiler should abort (or warn) if an argument doesn't have a field key
> ---------------------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-339:
---------------------------------
Fix Version/s: 0.4
(was: 0.3)
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.4
>
> Attachments: default+neg-field-2.diff, default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Attachment: thrift-339-5.patch
Fixes repeated fields in thrift_spec
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Patch Info: [Patch Available]
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696593#action_12696593 ]
Bryan Duxbury commented on THRIFT-339:
--------------------------------------
Is this something we could/should commit in 0.1? If so we need to make a final decision today.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) functions with default values broken
by #242 (r734536)
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672781#action_12672781 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
Although I'm happy to admit any breakage cause :-) I don't think this issue is valid (or at least, not for the reasons exposed). Note that you're not using a field key, and thus the thrift_spec variable is not populated at all. You can fix this using a positive field key:
{{ service Test
bool get_slice(1:i32 start = -1), }}
> functions with default values broken by #242 (r734536)
> ------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Priority: Blocker
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12674795#action_12674795 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
I had to rewrite my latest patch to fix structures with negative field keys, if they end with something lower than -1 For example, the ThriftTest.testException method generated a thrift_spec like this:
thrift_spec = (
(-2, TType.STRUCT, 'err1', (Xception, Xception.thrift_spec, Xception.thrift_limits), None, ), # -2
)
which caused some errors (i.e. thrift_spec[-2] didn't exist)
It's not as clean or pretty as the previous patch :( but it works.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alexander Shigin updated THRIFT-339:
------------------------------------
Attachment: default+neg-field-2.diff
sync patch with head
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.2
>
> Attachments: default+neg-field-2.diff, default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alexander Shigin updated THRIFT-339:
------------------------------------
Attachment: default+neg-field.diff
Another version of my patch + THRIFT-105.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-339) THRIFT-242 is
incompatible with arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696743#action_12696743 ]
Alexander Shigin edited comment on THRIFT-339 at 4/7/09 1:53 PM:
-----------------------------------------------------------------
{code}
bool get_slice(i32 start = -1),
{code}
start has field id == -1
{code}
bool get_slice(1: i32 start = -1),
{code}
works fine
was (Author: shigin):
{code}
bool get_slice(i32 start = -1),
{code}
start has field id == -1
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.2
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) functions with default values broken
by #242 (r734536)
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672721#action_12672721 ]
Jonathan Ellis commented on THRIFT-339:
---------------------------------------
damn it, jira said {{ }} would monospace my code samples there.
at least they are short enough that it's still pretty clear what is going on.
> functions with default values broken by #242 (r734536)
> ------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alexander Shigin updated THRIFT-339:
------------------------------------
Attachment: (was: default+neg-field.diff)
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Reiss updated THRIFT-339:
-------------------------------
Affects Version/s: 0.1
Fix Version/s: (was: 0.1)
0.2
Negative field ids are deprecated, so this doesn't need to block the release.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.2
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12683629#action_12683629 ]
Jonathan Ellis commented on THRIFT-339:
---------------------------------------
THRIFT-361 was applied. Does that mean we can apply patch #5 for this now, per David's comments above?
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12680404#action_12680404 ]
David Reiss commented on THRIFT-339:
------------------------------------
Esteve: We should be able to go back to patch #5 if my solution to THRIFT-361 is accepted. Thanks for uncovering this bug!
Alexander: At the moment, I prefer the solution in Esteve's patch (I am also biased, because I tried to push him toward that implementation). The main reason is that it is very low impact. The old generated code will continue to work with the new extension and vice versa. This version allows you to simply index into the thrift_spec list with the field id and have it work (without having to add an offset). The limits are completely unnecessary as long as the data is assumed valid. Finally, it allows users to set a field to None in the constructor, overriding the default.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12674346#action_12674346 ]
David Reiss commented on THRIFT-339:
------------------------------------
Something isn't right here. I think it has to do with resetting the sorted_key_pos inside the inner loop.
For this structure...
{code}
struct AllPos {
2: i32 foo;
3: i32 bar;
}
{code}
I get this output...
{code}
thrift_spec = (
None, # 0
None, # 1
(2, TType.I32, 'foo', None, None, ), # 2
None, # 0
None, # 1
None, # 2
(3, TType.I32, 'bar', None, None, ), # 3
)
{code}
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alexander Shigin updated THRIFT-339:
------------------------------------
Attachment: thrift-python-defaults-v2.patch
1. Oops... I have to sleep more. I've fixed the bug.
2. I can't imagine a scenario which makes you to pass None (instead of empty list/set/dict) to init. Can you give me one?
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) the compiler should abort (or warn)
if an argument doesn't have a field key
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672793#action_12672793 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
Argh, I just realized that even generating the thrift_spec variable, it wouldn't work as it would try to access thrift_spec[-1]
> the compiler should abort (or warn) if an argument doesn't have a field key
> ---------------------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-339) the compiler should abort
(or warn) if an argument doesn't have a field key
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672793#action_12672793 ]
esteve edited comment on THRIFT-339 at 2/11/09 1:39 PM:
------------------------------------------------------------------
Argh, I just realized that even if we always generated the thrift_spec variable, it wouldn't work as it would try to access thrift_spec[-1]
was (Author: esteve):
Argh, I just realized that even generating the thrift_spec variable, it wouldn't work as it would try to access thrift_spec[-1]
> the compiler should abort (or warn) if an argument doesn't have a field key
> ---------------------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696697#action_12696697 ]
Alexander Shigin commented on THRIFT-339:
-----------------------------------------
I can be wrong, but Esteve's patch doesn't work with old generated code. I believe it's a problem (because we don't break backward compatibility in python library for 0.1).
The big chunk of my patch was reviewed by Mark Slee in THRIFT-105.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Fix For: 0.1
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672808#action_12672808 ]
David Reiss commented on THRIFT-339:
------------------------------------
I think it was a tuple for performance reasons. I just thought of a way that we might be able to simplify both this issue and THRIFT-105. tuples can take negative indexes (indices?) just like lists, and they are counted from the end. What if we just extended the thrift_spec tuple so that all of the negative fields were in the right place (after all of the positive fields)? So if you had a struct like
{code}
struct foo {
i32 bar;
i32 baz;
2: i32 qux;
}
{code}
then the thrift_spec would look like
{code}
(
BLANK, # 0 aka -5
BLANK, # 1 aka -4
spec_for_qux, # 2 aka -3
spec_for_baz, # 3 aka -2
spec_for_bar, # 4 aka -1
)
{code}
This would not disturb the existing thrift_spec fields at all, so the current fastbinary extension would work fine. It should be robust against adding new negative fields to the end, since the indexes (indices?) of the existing fields will not change. I think it would make the patch for THRIFT-105 a lot simpler too, though we might need a little bit of magic to make sure that negative indexes work in the C code.
Thoughts?
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-339) functions with default
values broken by #242 (r734536)
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672781#action_12672781 ]
esteve edited comment on THRIFT-339 at 2/11/09 1:18 PM:
------------------------------------------------------------------
Although I'm happy to admit any breakage cause :-) I don't think this issue is valid (or at least, not for the reasons exposed). Note that you're not using a field key, and thus the thrift_spec variable is not populated at all. You can fix this using a positive field key:
service Test {
bool get_slice(1:i32 start = -1),
}
I think the compiler should abort if it doesn't find a valid field key and don't generate any code, though.
was (Author: esteve):
Although I'm happy to admit any breakage cause :-) I don't think this issue is valid (or at least, not for the reasons exposed). Note that you're not using a field key, and thus the thrift_spec variable is not populated at all. You can fix this using a positive field key:
{{ service Test
bool get_slice(1:i32 start = -1), }}
> functions with default values broken by #242 (r734536)
> ------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Priority: Blocker
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12673067#action_12673067 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
Thanks! I think that information could be stored as the first member of the thrift_spec tuple (key 0), as it's already empty. It shouldn't take too much work (as it's already in the sorted_keys_pos variable). What do you think?
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12673085#action_12673085 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
Ouch, you're right. A new class attribute (e.g. thrift_tag_limits) for storing maximum and minimum field keys sounds good, but I worry we might end up having too many attributes in the future and I'd like to keep all this information in a single attribute. Anyway, I think I can implement it in a relatively short time.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alexander Shigin updated THRIFT-339:
------------------------------------
Attachment: thrift-python-defaults.patch
The patch fix default values. The patch is depend on THRIFT-105 patch. The patch duplicate default values in constructor and thrift_spec. I found it far better compare to thrift_spec[xxxx][4].
The patch is small and (I believe) clean enough.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-339:
---------------------------------
Fix Version/s: (was: 0.5)
I'm deprioritizing this for now because I'm not sure what the status is, or if there's a champion for it.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Python - Compiler
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Attachments: default+neg-field-2.diff, default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696719#action_12696719 ]
Alexander Shigin commented on THRIFT-339:
-----------------------------------------
The Esteve's patch tries to fix two (THRIFT-105 and THRIFT-339) issue.
The fix of fastbinary protocol requires three argument instead of two. The change breaks all old-generated code if you use fastbinary even if you hasn't got any fields with negative tags.
The fix from THRIFT-105 (oh, it was six months ago) works fine with old-generated code, but it makes natural fields order: (-1, 0, 1, 2). Esteve uses order (0, 1, 2, -1).
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Fix For: 0.1
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696699#action_12696699 ]
Jonathan Ellis commented on THRIFT-339:
---------------------------------------
> I can be wrong, but Esteve's patch doesn't work with old generated code
I'm confused. The whole point of this ticket is so we can replace old, broken generated code with generated code that works. If you are regenerating what is there to not work with?
Personally I would be most comfortable to get Esteve's buy-in on whatever fix we go with since he was the author of the -242 patch IIRC.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Fix For: 0.1
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12680426#action_12680426 ]
Esteve Fernandez commented on THRIFT-339:
-----------------------------------------
Alexander: I don't fully understand your patch. Actually, it breaks the compiler and default values are not properly handled. For example, given this structure:
{code}
struct Foo {
1:optional list<i32> l = [1,2,3,4],
}
{code}
the code generated after applying your patch is:
{code}
def __init__(self, l=None):
if l is None:
l = [
1,
2,
3,
4,
]
{code}
which is wrong, which makes it impossible to pass None as l to Foo. Also, it doesn't set self.l to l, but I guess it was just a slip. With the current behavior, the generated code is:
{code}
def __init__(self, l=thrift_spec[1][4],):
if l is self.thrift_spec[1][4]:
l = [
1,
2,
3,
4,
]
self.l = l
{code}
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Attachment: thrift-339-6.patch
Fixes an issue that with negative field keys, all tests pass.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) functions with default values broken
by #242 (r734536)
Posted by "Jonathan Ellis (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Ellis updated THRIFT-339:
----------------------------------
Priority: Blocker (was: Major)
> functions with default values broken by #242 (r734536)
> ------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Priority: Blocker
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-339:
---------------------------------
Fix Version/s: 0.5
(was: 0.4)
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.5
>
> Attachments: default+neg-field-2.diff, default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Attachment: thrift-339-2.patch
Argh, I forgot to add the changes I made to fastbinary.c in the previous patch, here they are.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12683659#action_12683659 ]
David Reiss commented on THRIFT-339:
------------------------------------
Esteve: Good call on 3 vs. -2.
Don't we also need to adjust type_to_spec_args in order to include the limits for nested structures?
Also, it is unfortunate that adding the limits broke compatibility with the old version of the extension module. It doesn't seem like it will be easy to maintain compatibility, so as long as we are breaking it, we might as well go wild. If you want to combine the field specifiers and limits into a single attribute, that would be okay.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Attachment: thrift-339-3.patch
This patch adds some checks on lower and upper field keys using a new attribute called thrift_limits, I tested it both with the native extension and the Python-based binary protocol
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Alexander Shigin updated THRIFT-339:
------------------------------------
Attachment: default+neg-field.diff
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-339) THRIFT-242 is
incompatible with arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12674795#action_12674795 ]
esteve edited comment on THRIFT-339 at 2/18/09 1:43 PM:
------------------------------------------------------------------
I had to rewrite my latest patch to fix structures with negative field keys, if they end with something lower than -1 For example, the ThriftTest.testException method generated a thrift_spec like this:
{code}
thrift_spec = (
(-2, TType.STRUCT, 'err1', (Xception, Xception.thrift_spec, Xception.thrift_limits), None, ), # -2
)
{code}
which caused some errors (i.e. thrift_spec[-2] didn't exist)
It's not as clean or pretty as the previous patch :( but it works.
was (Author: esteve):
I had to rewrite my latest patch to fix structures with negative field keys, if they end with something lower than -1 For example, the ThriftTest.testException method generated a thrift_spec like this:
thrift_spec = (
(-2, TType.STRUCT, 'err1', (Xception, Xception.thrift_spec, Xception.thrift_limits), None, ), # -2
)
which caused some errors (i.e. thrift_spec[-2] didn't exist)
It's not as clean or pretty as the previous patch :( but it works.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Alexander Shigin (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12696743#action_12696743 ]
Alexander Shigin commented on THRIFT-339:
-----------------------------------------
{code}
bool get_slice(i32 start = -1),
{code}
start has field id == -1
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Affects Versions: 0.1
> Reporter: Jonathan Ellis
> Fix For: 0.2
>
> Attachments: default+neg-field.diff, thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339-5.patch, thrift-339-6.patch, thrift-339.patch, thrift-python-defaults-v2.patch, thrift-python-defaults.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Summary: THRIFT-242 is incompatible with arguments with empty key fields (was: the compiler should abort (or warn) if an argument doesn't have a field key)
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-339) THRIFT-242 is incompatible with
arguments with empty key fields
Posted by "Esteve Fernandez (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteve Fernandez updated THRIFT-339:
------------------------------------
Attachment: thrift-339-4.patch
Doesn't fix anything, but makes the compiler source code a bit simpler and cleaner.
> THRIFT-242 is incompatible with arguments with empty key fields
> ---------------------------------------------------------------
>
> Key: THRIFT-339
> URL: https://issues.apache.org/jira/browse/THRIFT-339
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Python)
> Reporter: Jonathan Ellis
> Attachments: thrift-339-2.patch, thrift-339-3.patch, thrift-339-4.patch, thrift-339.patch
>
>
> Esteve's last change to how default values are stored broke stuff. Here is a quick example:
> {{
> service Test
> {
> bool get_slice(i32 start = -1),
> }
> }}
> generates
> {{
> class get_slice_args:
> thrift_spec = None
> def __init__(self, start=thrift_spec[-1][4],):
> self.start = start
> }}
> which is obviously invalid.
> I'm not sure how thrift_spec is supposed to be populated here so I'm unsure how to fix this.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.