search

alan-if / alan-docs

Alan IF Documentation Project
https://git.io/alan-docs
Other
4 stars 0 forks source link

ALAN Manual: Container Limits #112

Closed tajmone closed 3 years ago

tajmone commented 3 years ago

@thoni56, I'm using this PR to track the changes proposed in #111 regarding Container Limits and the need to mention how nested containers are treated.

Before merging into master, we should discuss the details and keep adding changes to the man-container-limits branch, until we're happy the contents are fine, then squash all commits into a single one before merging.

I'm setting the PR as a "draft", and you as its reviewer, so that we don't accidentally merge it in unfinished state.passages.

thoni56 commented 3 years ago

Also, I noticed that the original BNF grammar allows also using THEN in place of ELSE, which is also not mentioned in the Manual (neither in the BNF rules nor the text).

(From memory:) I think the initial grammar used THEN but as that is a positive word, used in IF, WHEN and CASE to indicate "do this now", it was exchanged for ELSE which indicates "negative" and goes more in hand with the "otherwise do this" of IF, CASE, CHECK and restrictions. So THEN is just in the grammar for the compiler for backwards compatibility. (But should probably be removed now.)

thoni56 commented 3 years ago

(I haven't done PR reviews in GitHub before...)

It felt kind of silly to review my own changes, and I couldn't figure out how to allow @tajmone to become a review of his own PR ;-) so...

I changed the three NOTES to normal paragraphs, it felt out of place to have three... I also added a WARNING about the interrupted execution after the section on Extract.

Could @tajmone have a look and perhaps concur that we can merge this now?

tajmone commented 3 years ago

It felt kind of silly to review my own changes, and I couldn't figure out how to allow @tajmone to become a review of his own PR ;-) so...

I know, it's odd. I can't assign myself as a reviewer because I'm the creator of the PR, so the rule seems that PR creators can't be reviewers. Strangely though, I can dismiss any open review, probably due to repo privileges. I need to read the docs about PR reviews.

Could @tajmone have a look and perhaps concur that we can merge this now?

I just wanted to polish the sentence "including any instances in containers that might be in the container that is added to", which I commented upon — but couldn't open a review for — so I'm not sure if you're seeing it, it's in the "File changes" tab.

I'll try and fix it now, it's just that I wasn't sure what the sentence is actually trying to say, it's a bit entangled.

After that, I'll squash all commits locally, add a clean message and force push before merging (squash-merging from the WebUI could result in a strange commit message, I've read many complaints on how it works).

thoni56 commented 3 years ago

I just wanted to polish the sentence "including any instances in containers that might be in the container that is added to", which I commented upon — but couldn't open a review for — so I'm not sure if you're seeing it, it's in the "File changes" tab.

I'll try and fix it now, it's just that I wasn't sure what the sentence is actually trying to say, it's a bit entangled.

I don't actually see that comment. I'll try to rephrase now and let's see if you think it's an improvement ;-)

tajmone commented 3 years ago

Seems Ready to Merge!

I don't actually see that comment. I'll try to rephrase now and let's see if you think it's an improvement ;-)

Yes, that nails it! much clearer.

I've squashed all commits in this PR branch into a single commit with the original message, so if you're OK with I propose to rebase-merge the PR!

PS: Sorry for the delay in replying, but a friend came to visit from abroad so we spent the whole day together.