-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
All the list is convert into string if '-' is present #229
Comments
Thanks for reporting the issue. Here's the reason for it: Your shell is stripping the quotes from the input to fire. As a workaround you can put quotes around the whole input: The place in the code where an improvement would be made is here Line 57 in b77f5ae
Also #226 (strict mode) will help in this situation once it's available (but its not implemented yet) because you'll be able to specify whether an input expects a string or a list and Fire wont surprise you with a different type. |
Thanks for your answer @dbieber ! |
I tried a modification but there are so many usecases... if --name=['hello-eei38223','342efw3'] if --name=['hello-eei38223','hello'] if --name=['hello-38223'] if --name=['hello-eei38223'] So maybe wait for the strict mode is a better solution than modification into |
I think we'll want both. We definitely want to improve this. |
Hello! |
@dbieber I believe that we have a working solution for this. As it stands, the current implementation relies heavily on the AST library to identify individual elements in containers and put quotes around them when necessary:
What we try to do instead, is perform this identification of individual elements manually, by considering some general delimiters (take for example, when in a list, a comma (,) when not in a string, generally denotes a demarcation between two elements). When the identified element is further identified as a String (we still rely on AST for this purpose), we put back the stripped quotes around it. This takes care of all special characters, including hyphens, that occur in arguments. Our current implementation passes parser_test.py easily, and with the exception of 1 type of case (which needs some discussion), parser_fuzz_test.py as well. I can send over a flowchart and pseudocode so that you can get a clearer picture of what I’m talking about (we made these as part of our uni project). Do let us know what you think of this solution! |
Thanks @stephenvincent27! I'd love to hear more. This is definitely an area we want to improve in Fire. |
@dbieber Thanks for your prompt reply! Here’s a rough flowchart for our implementation: Drive Link To summarize: We parse the argument by dividing it into one of the three major cases:
Our general delimiter set consists of a comma (,), a space (‘ ’) and all possible closing braces (‘)’, ‘}’, ‘]’). Adding space to the delimiter set ensures that variable spaces added by the user are taken care of. Now, as I had mentioned earlier, we fail test cases of a specific type in fuzz tests, one example of which is: INPUT: K,\x1d Here, because of the comma in between, our implementation treats it as two different elements, and when passed to AST, it converts it into a tuple. So many additions leads to a high edit distance between the input and output strings, hence the failure. Again, this can be easily avoided by checking whether we are currently in a container or not. However, we were unsure about which behavior is more appropriate. Do let us know. There are two other test cases where failure is declared (again, due to high edit distances), but I think they can be ignored:
|
This is looking good! I agree we can ignore most of those failures of the fuzz test (not sure about what we should do for '...'; see end). We should update the fuzz test so those don't count as failures any more. I think our next step should be to list out new test cases so that we can some concrete examples on which to evaluate your approach. This will make discussing potential changes easier, give us test cases to prevent regressions, and will also help with describing parsing in the documentation once the change is released. We should be sure our examples cover nested lists, tuples, sets, and dicts, strings, bools, ints, floats, complex numbers, quoted and unquoted strings with and without non-alphanumeric characters. Here are two proposed tests; feel free to suggest more:
What do you think we should do with ...? Do you think we should treat it as Ellipsis or a string? I think a string might be better as it would be surprising to many users for it to be treated as Ellipsis. |
I do have some cases myself, I think we can add them to parser_test.py. I’ll suggest more when it comes to me:
Other than that, yes! We can update fuzz tests to accommodate our implementation. I had no idea how that formula for maximum permissible edit distance came to be in the first place, so kindly guide me on how to change it accordingly. Our implementation does cover every data type that you mentioned, along with other remaining data types in the Python documentation as well (as long as AST can parse them correctly). Regarding ‘...’: I honestly didn’t know that such an object existed until I came across one in tests; however I do agree that converting ‘...’ to ‘Ellipsis’ would be unexpected by users. We’ll treat it as a string, all it would take is an extra condition in the code. |
If all seems good, should we go for the PR? We can further discuss things there. |
Yes, that sounds good. |
Hello,
I reuse the introduction example code to illustrate.
When you give a list as arg, and into this list there is a string including this specific charactere '-', Fire turns all the list into one and unique string 😢. As you can see with the capture.
I'm looking into the fire code to see where the modification could be.
Thanks !
The text was updated successfully, but these errors were encountered: