the-carlisle-group / Acre-Desktop

A simple Dyalog APL IDE plugin that introduces "projects" and allows you to keep your source code in Unicode text files.
MIT License
11 stars 1 forks source link

SetChanged saves variables when a namespace is passed as argument #218

Closed aplteam closed 4 years ago

aplteam commented 4 years ago

The documentation says that SetChanged is one of the means to save variables even when the config file reads Variables=off, but I think that should be the case only if a variable is passed as the argument, or part of the argument.

When the argument is a namespace then the config settings should rule whether any variables inside that namespace should be saved or not.

Currently it is saving all variables as well.

aplteam commented 4 years ago

It might be a good idea to add a switch -vars in case one wants to save all variables as well.

norberturkiewicz commented 4 years ago

If I remember correctly, SetChanged was the only way to force a variable to save when it variables were set to off in the config.

I'm for the behavior being consistent with the switch. I would however like a warning that there are items that are not being flagged and tracked in the namespace.

What should happen if you SetChanged on a variable path but it's disabled in the config? Error and force a switch to override the behavior?

aplteam commented 4 years ago

I am moaning only about the special case that a namespace is provided as the right argument.

When a variable is passed to SetChanged then it saves it, no matter what the config file is saying, and that's perfect. If you don't want that, don't do it.

But if I want to save all functions and operators within a namespace but not any variables, then this is currently not possible.

That's why I suggest to change the behaviour so that it's rules by the config file.

PhilLast commented 4 years ago

This echoes #211 almost exactly and I don't feel qualified to decide the issue. I'd like an executive decision. But there is one side issue that I'd like clarified.

However, let's leave this open because at the moment SetChanged is supposed to save variables, but it's not actually doing this with -recursive.

This seems to contradict the whole purpose of the issue but I see no evidence of it.

aplteam commented 4 years ago

Seems as if back then there was a problem with variables but not anymore.

aplteam commented 4 years ago

I forgot about #211 but run into this again while working on a project. This time is was a variable that was accidentally not localized. I still have the strong feeling that SetChanged should honour the configuration settings.

PhilLast commented 4 years ago

@PaulMansour Can you decide this one way or other.

Editing a previously unsaved variable only results in its being saved if the Variables config setting is On.

SetChanged ignores the setting and saves everything it's asked to but requires the -recursive flag to save all members of a namespace tree.

Kai is asking for the latter situation to be changed such that SetChanged nnn -recursive saves all but variables unless Variables is On.

The matter is discussed at length here and in issue #211.

Editing a previously saved variable results in its being saved regardless.

PaulMansour commented 4 years ago

I'm wondering if we shouldn't get rid of the Variables parameter, and have it be On all the time. I'm sure way back when I thought it was important to not save variables automatically, but now with Git and they way I review may changes before committing, it seems unnecessary.

aplteam commented 4 years ago

I could no disagree more.

Originally I was in favour of saving all variables, and in the early days it did. Gil argued strongly against this, and after a couple of month I realized that he was right: I never forgot to save a variable I wandet to save, but many times I discovered variables that got saved without my intention.

PaulMansour commented 4 years ago

I assume saving a variable without intending to happens when you edit a variable that does not exist on disk correct? And probably a local variable when tracing? I guess I rarely use the editor to edit local vars.

I assume the editor has no way of knowing if the var is local?

aplteam commented 4 years ago

Yes, it's about local vars. It might be possible to work something out with ⎕STATE but I am not sure how trustworthy that would be these days.

On Thu, 20 Feb 2020 at 20:03, Paul Mansour notifications@github.com wrote:

I assume saving a variable without intending to happens when you edit a variable that does not exist on disk correct? And probably a local variable when tracing? I guess I rarely use the editor to edit local vars.

I assume the editor has no way of knowing if the var is local?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/the-carlisle-group/Acre-Desktop/issues/218?email_source=notifications&email_token=AD5XJHOCBY3XVJW7LL26263RD3OYNA5CNFSM4KXX66RKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMP3W2I#issuecomment-589282153, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD5XJHK5RSKIYRPCQ2VGNODRD3OYNANCNFSM4KXX66RA .

PhilLast commented 4 years ago

It's possible to identify locals with a lot of messing about and a fair degree of certainty within tradfns; dfns less so.

But I'm still unsure under what circumstances you might use SetChanged on a namespace which contains a currently suspended function. Adding a namespace to a project is something you're only going to do once. It's worth taking a bit of care over it. And if you do make a mistake you can always use ]Erase or erase its file in the OS.

As to the idea of SetChanged never saving even named vars without Variables←'On' I'd rather have to erase the occasional stray var that than make the mistake of crafting an array of important static data only to lose it because of an oversight.

aplteam commented 4 years ago

As to the idea of SetChanged never saving even named vars without Variables←'On' I'd rather have to erase the occasional stray var that than make the mistake of crafting an array of important static data only to lose it because of an oversight.

Fine. Bute why would you not have vars=on then? I don't get it.

You can actually achieve what you want. I can't because the behaviour is inconsistent. That seems wrong to me.

On Thu, 20 Feb 2020 at 22:20, PhilLast notifications@github.com wrote:

It's possible to identify locals with a lot of messing about and a fair degree of certainty within tradfns; dfns less so.

But I'm still unsure under what circumstances you might use SetChanged on a namespace which contains a currently suspended function. Adding a namespace to a project is something you're only going to do once. It's worth taking a bit of care over it. And if you do make a mistake you can always use ]Erase or erase its file in the OS.

As to the idea of SetChanged never saving even named vars without Variables←'On' I'd rather have to erase the occasional stray var that than make the mistake of crafting an array of important static data only to lose it because of an oversight.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/the-carlisle-group/Acre-Desktop/issues/218?email_source=notifications&email_token=AD5XJHPI5WL6LTA7NK5FZNTRD36ZVA5CNFSM4KXX66RKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMQTXJI#issuecomment-589380517, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD5XJHOND3RXSRNHKRRZAUTRD36ZVANCNFSM4KXX66RA .

PhilLast commented 4 years ago

If I create a project using defaults then spend half an hour creating a text array in the editor then log off I've wasted half an hour.

If I create a project using defaults then copy and add a namespace without checking whether it contains any stray variables of which I am previously unaware I can erase them.

Whichever way this falls there will be easy routes to correct any problems arising. As things stand everything is as described in the ocumentation.

PhilLast commented 4 years ago

After a long off-line discussion with Kai on this topic with both parties throwing up examples and counter examples I'd like to put forward this mutually agreed proposal.

CreateProject will continue only to add existing variables from the project space to APLSource if the -Variables=On option is set.

But whereas the value of that option is currently preserved as the homonymous configuration parameter Variables←On|Off no such config parm will be created or recognised subsequently.

Editing a previously unsaved var will always be ignored as there will be nothing to tell acre to add it. This will solve the problem of inadvertently saving edited local variables from the tracer.

SetChanged and EditArray really will be the only ways to add new vars to an existing project and they will always do it as there will be nothing to tell them not to.

Editing a variable that already has an associated .apla or .char* file will continue to cause the new value to be saved.

Please indicate your pros or cons asap.

e9gille commented 4 years ago

I started this reply this morning, but kept getting interrupted throughout the day so haven't managed to complete this until now. As I type this I see another message from Phil, so will post this as is and reflect on his message.

As Kai points out, I argued against saving arrays automatically unless explicitly saved. The reason was that—unlike functions—an array is typically variable and should therefore not be stored. Variables got saved to disc accidentally when I traced code and found myself editing variables to test edge conditions. Having the option to turn off the tracking of variables on a project level I think is great.

I haven't used the SetChanged command on a namespace, so I'm not sure what the scenario is. SetChanged is a command to flag items that would otherwise be overlooked due to them being changed outside the editor. If you use tools that change a large number of items in your project and you want to trigger acre to do its business then I agree with Kai that it should consistently ignore variables if tracking of them has been turned off for the project.

If the scenario is as Phil suggests, that you extend the project by copying a namespace from elsewhere that you'd like to add, then I can see that you may want to add arrays as well for that space.

PhilLast commented 4 years ago

Changes are ready to commit and push.

The only differences you'll see are

SetChanged and EditArray will continue to add new vars. Previously saved vars will continue to be saved when changed in )ED and ⎕ED.

aplteam commented 4 years ago

Decided: by design