Closed jrbeaumont closed 7 years ago
Looks good! Shall I merge?
Thank you :) I will go through and tidy up and make everything under 80 characters and merge it myself. I wanted your approval on the change, but there's no need to wait for me to tidy it. Thank you very much
:+1:
By the way, I've just demoed Plato at Dialog -- everything went well :-)
That's great to hear!
As long as you didn't find any bugs then we're winning! Haha.
I hope they have good and constructive feedback.
The only issue is that for some reason images in your blog were distorted (incorrectly scaled). It was an older Windows machine with IE browser. I checked and everything is fine on my machine.
I just checked on the standard IE on the university computers and that does not scale them correctly either. Every other browser I have tried does. I will look into this and see if there is an easy fix.
@snowleopard: I'm gonna go through and tidy up all code so it is 80 characters or less, as in some cases I have found it might make some things clearer. Is this OK? Would you mind reviewing before I merge?
I've added some comments -- in general I feel you are starting to overuse point-free notation: the code is difficult to follow and may be hard to maintain :-)
I will fix these, and then have a review of all the code, maybe trying to reduce the point-free notation, hopefully clearing some things up.
@snowleopard I have changed those. I will have a look through for some point-free notation, but if you spot anything, please let me know.
Yeah, I am struggling to understand where and how to improve. If you could point to some problematic functions I will have a crack at clearing them up, seeing if I can find others in the process.
@jrbeaumont Functions removeDupes
and addMissingSignals
could benefit from being rewritten in a clearer way, even if they will become longer as a result.
Note that in general if the line is getting too long, it is a good indication that perhaps some intermediate terms/functions should be defined in order to clarify the code.
In many instances where you simply broken lines into multiple lines, I would instead consider coming up with a clear decomposition of the line into simple steps.
@snowleopard: The latest commit to this branch will remove redundant arcs produced by sequence
. Testing proves this works correctly, not affecting the previously working concepts, but using the example of an ORAND the STG produced one has two c-
transition objects.
Feel free to review and offer tips for improvements on the implementation. I tried to write these changes in a form that clear, let me know what you think.
This PR is not quite ready, I still need to tidy up the code you suggested earlier. I also forgot to try and shorten code in Tuura.Plato.Translation
so I will fix this while I am at it. Should be done soon.
@snowleopard: I have cleared up everything I can. Some further revisions may be necessary on one or two functions but I need to discuss these with Adrian.
Please let me know if you have any further suggestions :)
@jrbeaumont Before merging this, could you add comments for each function? It may be difficult to understand what's going on in a few months from now.
Sure thing. Each and every function, or just ones that are quite complicated?
Well, if a function does something completely obvious then you can omit the comment (although in this case the comment is also easy to write and probably won't hurt).
OK, I'll add some comments and you can have a quick review before merging :)
@snowleopard I have added the comments into the flip
branch in #76 so, so these functions include comments too. Please review them there.
OK, I think we can now merge this one, right?
Yes, please go ahead.
All causality is now stated as
Causality [e] e
instead of being split intoAndCasuality
andOrCasuality
.AND-causality will contain single element lists as the cause transitions, e.g:
rise a ~> rise c <> rise b ~> rise c
will beCausality [a+] c+
andCausality [b+] c+
OR-causality will utilise this list format as previously, e.g.:
[rise a, rise b] ~|~> rise c
will beCausality [a+, b+] c+
Testing has proved this continues to work correctly.