Rewritten answers file writing/reading. #37
Conversation
1. The answers file contains no more (y/n) confirmations. 2. Reading the answers file doesn't ask for confirmation. 3. Reading the answers file doesn't repeat questions again, exits with an error if there were problems. 4. Added a marker for the end of answers file and checking for its presence. 5. Reading the answers file doesn't print hints. 6. Fixed the echo of the lines read from the answers file.
|
There are some broken error codes to fix and error messages to improve. |
|
Disclaimer: I didn't really look at the code (though I see that you wrote to the answers file after everything was done and that could work) and there could be a language barrier between us.
The problem here is if it doesn't have the confirmations then the answers file will not work because the questions have to be answered to proceed; if the answers file no longer has them then how can the questions be answered?
I don't know what you mean by this: the only code that asked for confirmation was the check functions that I simply added answers to. You can add the answers later of course but the answers file didn't ask for confirmation; the tool did.
Same as above.
Then if they change their answers (or files that no longer have these problems) they won't be alerted to a possible problem. When something is automated at the same time as warnings being ignored the user should be alerted to the possible problem. I would argue that whenever warnings are ignored for this contest they should be alerted to a possible problem. People are used to automating things without thinking and this could result in problems.
If this is what I think it is I like that. |
|
Oh and answers errors should be there because if there are I/O errors on the answers file then they obviously cannot use the answers file and expect valid results. Removing that check means that the user will not be alerted to possible problems. But maybe you added that later on and I didn't see (I just took a quick glance). |
|
We think @xexyl has made some valid points above. Moreover we are not sure that avoiding the questions is a good thing. It certainly complicates the code to try and avoid them. When we just had the "-a answers" feature, one could do this: ./mkiocccentry .. < answers We suspect that any "-i answers" mode should simply be another way to accomplish feeding the answers file into stdin. Yes, if someone were to edit the answers file, such as supplying a sting that is too long, or too short, etc. one would get the desired result when getting that modified answers file back on stdin (or via "-i answers"). But we don't think the code should be complicated to try and handle such a case. Please recall that this mkiocccentry code is something an IOCCC contestant would run a few times per year. We, the IOCCC judges, have to do a lot of work for the number of entries we do receive for a given IOCCC. That is why we limit try and limit the number of entries someone can submit for a given contest. And of course the submitter has to do a huge amount of work to just write also those entries to feed into mkiocccentry! While it is fun to polish the code, using mkiocccentry efficiently might not be a significant goal to focus on. [[see our next comment in this thread for what we are focused on now]] While we are still pondering this issue, we suggest that it might be better if: ./mkiocccentry -i answers work_dir ... Behave exactly like: ./mkioccentry work_dir ... < answers And NOT try to avoid the questions and prompts and hints, maybe? Something to ponder. Comments welcome. See our next comment for our current focus. |
By confirmation, I mean asking for yes/no.
No. The user receives a warning with a list of problems at the end.
The log becomes a complete unreadable mess without this fix.
If someone wants to use this they can use And if there is exactly the same as stdin copy, then I don't see the point of mkioccccentry having the
This is used as a form of confirmation to ensure that the answer file was read correctly. |
|
The reason I hide "hints" is because these hints are too over the top and annoying. When using automation like this, I don't want to scroll through multiple screens of things I already know looking for warnings. So I printed out a list of warnings at the end. So I can just look at the last screen in the console without scrolling to see if something bad happen or all is OK. If someone is using saved answers, that means that person has already read all of these explanations. |
I see that now as I tried it. But there was no need to make this change. Functionally you removed the warnings (and warnings are good to have) and didn't do anything I didn't do (well you added the EOF marker but I mean the results are the same); you just went about it a different way. You give warnings at the end of the output rather than in the middle of it. I had warnings too and warnings are important.
This doesn't mean it's not good to have it built-in. I already gave reasons why it's good to have it built-in. Just because someone could use tee doesn't mean that it's not good to have it built-in.
Then why even work your changes if you don't see the point? I already gave reasons it's good to have it built-in. It takes the burden from the user: you can rely on the tool itself rather than a pipeline. Also tee will happily overwrite the file and that's easy enough to do if you do it quickly or without thinking. By having the options it ensures that this won't happen. |
I think this is a problem with you not being able to focus on the important bits - the warnings are important for users who ignore warnings. How far do you go? One could use your logic (below) that if they have seen it once then you shouldn't have the warnings of possibly invalid files at the end when using the answers file.
You have far too much faith in users if you think like that! Besides that it has to be done at first for them to know it. But if you think people will remember it or they won't make a mistake when ignoring warnings you don't know users well enough. As a game developer the things I saw... You're not likely to have to refer to the warnings because you're not likely to submit those files that trigger warnings anyway. And having warnings is always good. Always. |
It already does. Before I added the second option that's what you could do (you could also use cat answers | ... but that's unnecessary use of cat). |
But trying it with Ilya's code using the option works okay; however with the < I get an error:
...so it doesn't even form the tarball. |
|
One more thing I noticed is it seems that once again fclose() on answer file is done before forming the tarball. Yesterday I moved the fclose() call to be after because freopen() makes stdin refer to the answers file and you can get bad file descriptor by closing the answers file as the function verify_entry_dir prompts for input. I did not notice this happening every time though. Unfortunately the git log didn't show up so all you see is that I had some minor fixes (I added the bad file descriptor fix after starting the other change). I specifically stated the above part though in the git log on my branch. |
|
Instead of this text being posted in this pull request, Please see this discussion: https://github.com/ioccc-src/mkiocccentry/discussions/39 Please also see these issues where help is wanted:
Thanks for your consideration and help! |
By "But trying it with Ilya's code" do you mean the code at the top of the master branch now, or do you mean as modified with the proposed #37 pull request as it is now? |
That certainly needs to be addressed ... (along with #37 (comment)) :-) |
No, warnings in the middle are shown just as before. |
With redirection to standard input without the -i option? You should not do this and should not expect it to work. |
My changes make sense because it's more than redirecting the answers to or from a file. It also makes it easier and prevents the answers from being misread if there a warnings happen/not happen. That's why it's in a slightly different format than just a copy of stdin. |
I meant his patch - #37. |
How do you mean? I already moved fclose() to after the form tarball function but in Ilya's patch it was moved back to before forming the tarball. Or do you mean I should make a commit so that there is a git log of it? |
Right but Landon suggested it has the same behaviour and yours doesn't. That's what I was saying; yours errors out because you removed the confirmations from the answers file. |
Mine makes just as much sense but has more warnings which is a good thing when ignoring warnings. The copy of stdin means one doesn't have to rely on the program but they can. It has the same behaviour. Your change does not make it easier: you still use the options along with the answers file. The difference is you omit confirming the warnings by the tool and that's not a good idea. But you still have to initially answer that you want to continue anyway - after that (using the answers file) you don't have to (just as with mine). The tarball is formed fine under most circumstances in your code but I gave one where it isn't because you changed the format (namely removing the 'y' lines). |
Since this is a fix, confirmations may appear in different numbers depending on warnings, but answers file will work in any cases now (with the -i option, using stdin is deprecated) and not depend on the warnings anymore. |
It doesn't need to be after tarball function now. It was needed just because confirmations, and the confirmations are not needed for the new answers format. |
|
I can make < work with the new format if you insist. By simply adding a header to the answers file. So if mkioccccentry sees this header in the first prompt(), then interpret it as an answers file from the "-i" option. |
|
You (@ilyakurdyukov and @xexyl) are having a good discussion. Based on some of your replies, we seem to have been mistaken when we wrote some of our previous comments. May we suggest that you try to arrive at some common recommendation if possible? For example does the pull request meet your common ideas or does it need to be further updated? |
That seems like an interest possibility. What are the downsides to doing that, if any? |
I want to include the version in this header, this might be a nice feature if something changes later in the tool. So the version in that header will also change and the newer tool will say that the answer file is in an older format and should be re-entered. But change the version in the header only if there are incompatible changes, keep the same if changes in the tool do not change the field order or introduce a new fields/questions. |
This would however also mean that you have to interact with the program. Yes it would be mostly automatic but it wouldn't be entirely. Is there a way to restore stdin after using |
|
We have to go to a meeting on a different topic. Good discussion. Please do consider updating the patch and let us know if you are able to reach some sort of consensus. |
We liked those warnings, BTW. |
I personally think it's better to work on the things that still haven't been done. Ilya seems to be experienced in JSON so he could go for that part. He could also change the warnings to go to stderr if he doesn't like them so he could redirect it to /dev/null (as I suggested several times). That sounds like a good compromise to me. What seems very important to me is to make sure to have those warnings though and also let the user decide whether or not they want to proceed. Ilya makes a point about how a different warning might be ignored because of the 'y' in the file but his patch doesn't let you decide at all - it just assumes yes for all warnings. But what can be done with the y ignoring a different warning? There IS a certain risk with ignoring warnings and there's no way past that. But it's better to give the user the chance to abort and tell warn them about this. |
I have a lot of experience with users. If you don't have warnings like this there will be people who complain! It's true that warnings might annoy some users but those users can simply ignore them; warnings like these are for the people who are actually doing the risky things. |
I'll try to do this (so you have the feature you want and I have the feature I want), deal? But it's in the morning, because it's already very late in my time zone. |
Well... as the judges liked my warnings I think they should be put back. I still also think it's imperative that the user gets to decide whether to ignore the warnings or not. I also think using shell redirection should have the same behaviour. Maybe we should have been a bit clearer with each other. I'll start: What exactly do you want? I can then reply to that though it won't be until my tomorrow as I go to bed early. Maybe we can come to some consensus.
Yeah I was thinking that. Sleep well! |
|
A possible way to restore stdin occurred to me. I've not tested this but doing something else entirely the dup(2) syscall popped into my head. I have no idea why but this happens with me. It does seem like it would complicate it unnecessarily though (though see below). Also why have change stdin to be a file only for it to go back to input at the keyboard? Is the purpose to make the user confirm everything is okay after the tarball has been formed? I can maybe see that as an idea - since they are relying on automation. But at the same time they can look at the output and see if it's correct. I don't know. I'm kind of mixed here. As I'm thinking on it this might actually be a fair idea. I don't know if it's necessary though and it would complicate things quite a bit. But Ilya it seems you're after certain things. I want to reiterate my question of what are those things you're after. I think that's the fair approach. As for what I want it's basically what I did. I will not ever ignore the warnings but I think having the warnings there is a good idea (as do the judges as they have said). If you want a header/version tag I think that that might be an idea as long as it behaves the same as And you're right that if the y is there it might inadvertently say yes to a different warning but as I noted ignoring warnings has a certain risk to begin with and there's no perfect solution (but it's better to not assume yes for all the warnings). What I am against is taking the choice of whether to ignore the warnings or not away from the user and I think that in addition to keeping that the user should be alerted to the fact that if they change the files (in a way that triggers - or no longer triggers - a warning) it might result in invalid input. My original idea was that I could simply input the answers once and then not have to repeatedly input the same thing. I would never ignore the warnings about e.g. empty prog.c (though interestingly I did have a different take on an empty prog.c in 2018 and I thought it was fun and funny but I knew it was highly unlikely that it would win ... I knew also that if any of my entries that year would win it would be the Weasel entry which of course it did) but I added this to the code though because I felt it should be complete - without it the answers file would not work (and you saw what happens if you change it from valid to not valid in your patch). So what are your wishes? I again stress that if you're not liking the extra warnings just make them go to stderr so you can redirect it to /dev/null. That would solve that part of the problem for you but it would still be there for people who are actually going to ignore warnings. And as for me (besides above) the more I think on it the more I wonder if it's a good idea to have the user have to actually type 'y' that confirms that the tarball is correct. It might be a bit annoying for me but it might be the better way to go about it for the contest as a whole. The dup() syscall is the only way I can think of doing this but again I've not tested this (maybe there's another way too). I think that's all I have for the day. Hope you're having a good sleep and a good day when you wake up! |
Would you mind defining 'problem'? Because it might be something that's implicitly there. If the input is invalid then the process will fail. But define invalid input. If it's an invalid sequence it will fail but how to tell if it works but is actually not correct. Ilya pointed this out with 'y' being there could make it ignore a different warning though they already are ignoring warnings which is risky but is there another thing you can think of? I think the idea of abort on any problems and issue a message that the user fix or re-run without the answers file is a sound idea but the question is (as above) how to define problem. |
FWIW I added in a copy of iocccsize.c in read_ch() an fprintf() call to print the character and this is what I got running it on mkiocccentry.c:
Running the 2020 iocccsize like:
Gave this:
Which I guess might be the diacritics in some of the countries? I did not do any debugging but maybe that will be something of interest to you or make you think of something in particular. |
|
I just tried removing the diacritics and I got the same warnings. I guess the obvious answer is to have the code print the adjacent characters so you can find out which ones are giving it trouble. I'm going for the night but maybe it'll give you some ideas. If nothing else it doesn't seem to be the diacritics (I assume those characters have diacritics) in the country names. |
So you're angry because I'm removing your warnings. But that warning is completely deprecated with the new method. Because: "answers file will cause invalid results" The answers will NEVER give wrong results with my method, either any warning are triggered or not triggered. I explained it, but you keep insisting that these warnings be returned. And why I strongly dislike the warnings you made, and are strongly against them: These warnings do not solve the problem, instead of solving it, you leave the problem as it is and think that this is a good solution? I think warning the user is not a solution. Is this something that can't be fixed? No, it can be! And I don't see your intent to make a real fix other than these warnings. You're happy not fixing the problem, but I'm not happy with that solution. So I made a fix, and you are angry at me for changing your code - is this the real problem, what are you against my patch? Not because anything else? I removed that warning because it's obsolete now, because the problem solved.
So, you want me to go do the other part and give up to fix this just because you don't want someone to edit your code, even if your code doesn't solve the problem and someone has a solution? Not fair at all. I think it's impossible to develop the code together when one of the participants is trying to keep their code untouched, even when it is outdated and there is a better solution. And they attack those who try to change their code, coming up with reasons why the new code is worse. |
The user is prompted for final confirmation if there were warnings when using the -i option.
Answers file can be read from standard input. Warnings and messages could be improved.
|
The answers file can now be read from stdin. And the user is asked for final confirmation if there are any warnings and the -i option is used. But it's clear to me that Cody refuses anyone to edit his code. That's the problem, because I offered a solution to all his complaints about this patch. This is very unconstructive behavior, because if we avoid changing other people's code, we won't be able to improve anything. His warnings should no longer be here because the answer file is now consistent and cannot be broken by the different warnings layout. |
|
The new code should be edited with additional comments, improved warnings and error messages, and fixed error codes. But I'm not in the mood to do it. Right now I'm very stressed and tired of this pointless conflict over the outdated warnings. |
|
I will try looking at the rest later but I wanted to say I'm not angry with you. I know you're sick right now and you should take it easy until you're feeling better. I know all too well how hard that can be to do but it's important! I don't want to contribute to you not feeling well. If this discussion makes you think I'm angry it's time to take a step back - which like I said in email might be good anyway. I hope you're feeling all better soon. |
|
Will try now to reply because it seems to be important. I want to reiterate with you I'm not angry at all.
Please don't make it all about me. This is one of the reasons I asked you what it is you want. Remember the judges also liked these warnings. I'm not saying they can't be improved but I think having warnings is important - yes even with your changes.
But that's the problem: the user should have the choice of deciding to confirm that they want to ignore the warnings before the tarball is formed. Those final warnings you have are good but they're not enough for all users, I can assure you that!
What's more important is that the user has the choice of confirming that they want to ignore the warnings - and in so doing it's why the warnings should be there. You have too much faith in users if you think the final warnings are enough!
Neither is a perfect solution: your way doesn't solve the problem either because it removes the choice from the user whereas mine it can cause invalid input which means the user must revisit the answers (because a change was made that warrants this).
It's not a solution. There is no solution to the user ignoring warnings. But warnings are there for a reason and if the user cannot confirm this then there's a problem.
I'm not angry and if you think I am you should take a step back. I know you're recovering from a cold and it's not good to be working yourself up when you're unwell (it's never a good idea but especially when you're sick). I've even suggested that some of your ideas are great and I gave a possible solution of how to do it (e.g. since I used You say the warnings are obsolete now but that's because you've completely removed the possibility to confirm that the user wants to ignore the warning which is not a solution at all. Once the tarball is formed if the user did not get a prompt about it some are likely to ignore the final warnings. Again I think the final warnings are a good idea but I don't think not letting the user decide is a bad idea too.
I'm not against all of your patch, no. What I was trying to say though is as the judges pointed out they could use more help on other parts - as the answers file is fine. Oh sure there are places it can be improved upon but removing the choice from the user is not one of those places. The judges have given us other things to do. As for bringing up JSON I did that because it seems your'e experienced with it - it was nothing but encouragement! Again if you're thinking like this you should take a step back (see above - I have agreed that some of your ideas are great even one that might be annoying to me but is a better idea than what I have nonetheless).
I'm not attacking you. I'm having a discussion with you. I also don't think your patch is entirely useless. We're disagreeing on one point in particular (and this point also leads to your removal of the warnings): that you think the user doesn't have to have the choice of confirming that they want to ignore the warning. That's the thing I'm against. The rest I'm not against. But as for the warnings I gave a possible solution to you (since it seems they annoy you). Please take good care of yourself and if you're stressed do take a break. I cannot stress that enough! It's so important. As I also said I realised yesterday I've been irritable (though not at any specific person - just a general thing) and that's a reason I wanted to step back too. It's also why I asked you what you exactly wanted. I hope this helps you out. I'm not against all of your changes. I was encouraging you on the JSON because you're experienced in it (and I'm not) and I know you'll do something great. I'm definitely not attacking you but if you think I am you should take a step back ... and I'll do the same. A clear head from both of us will help the matter. Feel better soon. The most important thing right now is that you take good care of yourself whilst you're recovering from the cold. Not programming. Not anything else. You have to take care of yourself first. You can address my concerns later (again though I'm not attacking you nor am I against all your patch or ideas - I'm against one part of it). |
That's good.
Great. I agree with this.
This is not true. I'm not against anyone editing my code. I have told you things I like about your ideas and patch. I'm only against one thing in your patch.
If you've removed the ability for the user to decide whether to ignore the warnings though this is not a better way. Yes it means that the warnings might not be relevant but it's riskier if the user is not PROMPTED. That's the problem. They should be prompted. Having a warning at the end is great (and I had one too) but if they're not prompted they're much less likely to think about it and that's not good. Well maybe it's good in that others are more likely to win :) but I'm trying to be fair to everyone. |
Please take good care of yourself. You're recovering from a cold. Doing that is more important. I'll give it a break too. Let me put it this way though: My main complaint is NOT about removing the warnings; it's the reason you removed them: not prompting the user if they want to ignore the warning. Humans have this tendency to ignore warnings but they're MORE likely to ignore warnings if they're NOT prompted about it: if they're not asked if they want to continue anyway. In your code they don't even get a chance to confirm that (in the checking of the files). It's that that I am against. This is not a pointless discussion (yes - discussion). |
|
Okay here's a thought. I might just have a good compromise (or even just fully concede). I just took a break - had a shower, had some food (well actually liquid - I don't eat until late in the day) - and I was thinking. I'm sorry I'm adding stress to your situation; that's not what I mean to do at all. I want you to feel well. Last night I slept about 11 hours and I'm still exhausted. Part of the problem is I had a lot of vivid dreams and dreams always exhaust me. Anyway I will not consider any of this today. Instead I'll take it easy. Tomorrow I have something going on but I'll make an effort to look at this Tuesday. It's true that I think it unwise to remove the prompting of the user about warnings (that's my problem with this patch) but since I won't be ignoring the warnings if it means that much to you I'm seriously considering just letting it go. I mean it's not the end of the world and it doesn't truly affect me or my entries. I do have a suggestion though with your warnings that might just solve the problem: maybe instead of including say prog.c is 0 size maybe have the full warning message that the code shows earlier on (in the check functions). That way the user has a better idea of the seriousness of it. That could be a good compromise actually as although it would still not have the prompt it would remove your problems with the confirmation too (which means the answer file will always work - which seems good to me I might add ... again I'm not totally against your ideas!). This way you have your way but it still expresses more warnings to the user. I'll probably look at the code before Tuesday but I will try not reacting to anything until then. How does that sound to you? I don't want to add to your stress and I'm sorry (it's very clear to me that I have) that I've upset you. I'm so serious about that that I'm almost tempted to just say: add the full warnings and then go for it. That might just be enough to make me happy (it's not as good, maybe, in some ways, as having the prompt, but it at least explains to the user why there's a problem rather than just the one line message). Hope you have a good night and hope you sleep well and again I'm very sorry that I've upset you. Btw I still think you should go for the JSON code irrespective of this code; I know you'll deliver a great tool! I know I wouldn't as I've never played with parsing JSON nor have I even played with JSON full stop. But it seems like you would do well. I'll take a look at the other requests later on - probably not today. Cheers and I hope you feel better soon mate! I hope this message helps you out. I really am trying to be reasonable and that's why I'm trying to come up with a solution (and why I've given it thought even when not at the computer). |
|
We will continue to watch your discussion @xexyl and @ilyakurdyukov. Let us know when you reach a consensus and update the pull request if/as needed. |
|
A short bit ago I had a thought that might help here. After that I looked at the code and saw that it's already there. But looking at the code I see a couple ways to improve it. I'd say go for accepting these and I'll add in my thoughts below. Here's what I like about the patch that's different from my code:
I did notice though that if you ignore a warning you can have the result of:
So reading from stdin is incomplete. Here's where I think there could be some improvements (besides making it so this is complete).
Since Ilya is stressed and also these are my summarised thoughts I'll add these. Thus I would say you should go ahead and accept these changes and I'll work on the additions. This I think will make us both happy and I think the changes together will make the tool all the better. How does that sound to you @ilyakurdyukov ? I hope this will make you happy and again I'm sorry if I caused you any stress. With a clearer head it's a lot easier to see your ideas as an improvement (I was against not prompting the user to ignore warnings and since you've added that - I did not see that yesterday as I was too tired - I think it's good but I still think I can update it a bit to make it a bit more rounded). I'm going to work on these changes now on Ilya's code and once this is accepted into the repo I will just copy the mkiocccentry.c in that directory to my fork and then I will do a PR. |
|
Okay I just did the following against Ilya's changes and I think this will work out; it'll be a good compromise between us and I hope it'll work for him.
He had it so that there's a summary of warnings that are displayed after processing the files. However the warnings that were in the check functions were also printed. When reading from the answers file he had it so that it did not prompt you: it just went right ahead and proceeded but still showed the warnings. What I did is make it so the full warnings are not displayed until the end (after processing) and prompt individually for each warning (as if it wasn't automated except that everything other than the warnings are automated much like how he had it only I show the full warnings to show the importance of it). If there aren't any warnings there's no problem: the creation of the tarball is done; else it's automated but at the end the user can decide whether or not to ignore the warnings. This means that those who don't have any warnings in their files can automate it without any prompting but if they do change it they will be prompted. This satisfies my disquiet about not having the prompts but it also means the answers file will always work (which satisfies Ilya's concern). I also modularised the warnings: each warning is in its own function which prints the warning and prompts if the user wishes to proceed. This way there's no need to duplicate the code in the two places.
I made it so even if it's automated (answers file used) the user is prompted if the directory is correct (correct files and so on). I think this is a good idea but this is certainly a matter of opinion: it could go either way and I'd be happy; whatever the general consensus is. I'm okay with this either way.
I think that given that the answers file has the tag and EOF marker that maybe it's not necessary that if there are warnings that doing
Once Ilya's PR is accepted I will commit and do another PR against his and I think we can both be satisfied.
I seem to be having a problem with my charger on my laptop; my power bank works but this could be a serious drain so I'm not sure how much I can do until this can be worked out. It was working strangely before but after a power outage it no longer charges at all so I need to get a replacement (and that might not be the problem because it seems also that there's a possible building wiring fault on the line). I hope to get to the other stuff sooner than later. |
|
This is great progress @xexyl and @ilyakurdyukov! Thanks! Let us know when the pull request has been updated / ready for us to merge. |
|
@lcn2 Please merge it. Once that's done I'll fetch upstream on my fork and then commit my changes and make a pull request. |
The answers file contains no more (y/n) confirmations.
Reading the answers file doesn't ask for confirmation.
Reading the answers file doesn't repeat questions again, exits with an error if there were problems.
Added a marker for the end of answers file and checking for its presence.
Reading the answers file doesn't print hints.
Fixed the echo of the lines read from the answers file.