Replies: 5 comments 4 replies
-
|
All good points, @xexyl Please prepare separate issues for each tool to be fixed. Feel free to open separate PRs for each rule to be fixed under those separate issues. |
Beta Was this translation helpful? Give feedback.
-
|
Okay I'll open an issue for each in a bit (I hope): that way things can be discussed as I have a number of questions and this seems like it's kind of OT here. |
Beta Was this translation helpful? Give feedback.
-
|
.. actually I see it's more complicated than that :( some functions check for 0 length and others have no check. So it seems this will take a bit of work. I don't have the time right now to open an issue. I might have to do that tomorrow. Unsure of this for the moment. It's an unfortunate problem but I'm glad I discovered it. |
Beta Was this translation helpful? Give feedback.
-
|
For those who might have wondered into this discussion: Please see issue #33 and update related to work on a number of the topics discussed above. |
Beta Was this translation helpful? Give feedback.
-
|
We believe we have addressed all of the current questions that still need answering at this time. If we've missed something or something else needs to be clarified, please ask again. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
(I will do a similar discussion for the guidelines - I hope to do that later today.)
I am not sure how you wish to deal with this. I can certainly do a pull request and rebuild them. Or you can fix them, Landon, if you choose. Here are some of the issues.
rules
Top of the rules
It should be 'important', not 'import'.
NOTE: this is also in rule 3!
Rule 3
'no' should really be 'number' to be more clear.
See also above where an earlier comment in the rules is also in rule 3.
Rule 15:
It's no longer a single email exchange so it might want to be updated?
Rule 17
This is almost certainly my fault but it would be better to be something like...
This is also my fault. I did not remember that this format was in the rules when I fixed it to be a bit less strict at your request.
This could be ambiguous. It could be better to have the last sentence as:
Next one also in this rule.
This last part does not seem to be true: in particular the first one: 100 characters in length for filenames.
It might also be good to mention that txzchk does this check (well the second one: the first one not but if we added it to a new function that tests filename it might be easy enough to do). This might delay things a bit so it could be that the rule suggests it but not say it is enforced. I'm not sure. It's also possible I forgot something but it seems not.
In fact there are only two cases in txzchk.c that use
strlen()and neither are for that purpose! The variable name in the filefilenamealso does not suggest it's being tested (in that way). It does check the POSIX safe + chars though.Either way the statement about the length of the filename seems to not be true!
It would be clearer to state that one may specify files they have in other directories in their system. One would like to believe it is clear enough but it might be ambiguous to some .. maybe.
As above this seems to not be true, at least not in the code.
This is checked in txzchk.c yes.
This is also checked.
Right - checked also.
Also checked. However to help simplify the rule it might be good if we merge the previous part of the rule that mentions only two of these (including one that seems to not be true) into this list so as to not only make it shorter but to also be slightly less confusing.
.. except for the 100 length of filename of course .. unless I'm missing some trick macro. In fact I don't even see 100 by itself in soup/limit_ioccc.h!
I believe that I wrote this but whether or no it might be good, if we're being thorough, to say that chkentry is run on the two JSON files prior to the tarball being formed, and that the tarball MUST have them!
End part of rules
Okay it might be unnecessary here but ... one does not need to have them: rather they should read them.
Also it uses the wrong term 'entries'.
Beta Was this translation helpful? Give feedback.
All reactions