Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deprecation warnings in preamble of FileSystem-Git package #258

Closed

Conversation

LinqLover
Copy link
Contributor

As mentioned here.

Copy link
Contributor Author

@LinqLover LinqLover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it appears impossible to do this via Squot, interesting ...
(In SquotBrowser, I saw the postscript indeed)

@j4yk
Copy link
Collaborator

j4yk commented Feb 11, 2020

Wait a minute, I might have renamed the instance variable from isInFilemode to isInFileMode back then. In that case this is the migration code and it must stay.

But if that is the case, the script could use an upgrade to be idempotent.

Do you mean Squot deletes the postscript on save? That would be a bug of course.

@j4yk
Copy link
Collaborator

j4yk commented Feb 11, 2020

Here it is: LinqLover@cf6fe5f

Looks like the change is okay then. Can you check again please? I just got up and should not review code in this state. :-)

@LinqLover
Copy link
Contributor Author

I don't see the problem, but I never actually faced with the implementation of Squot :) The postscript should be executed after loading the latest version, so #isInFileMode has been installed and can be safely used, can't it? The postscript does not touch the instance variables!

@LinqLover
Copy link
Contributor Author

Do you mean Squot deletes the postscript on save? That would be a bug of course.

Need to reproduce this.

@LinqLover
Copy link
Contributor Author

Hey @j4yk, any updates on this PR? :-)

@j4yk
Copy link
Collaborator

j4yk commented Oct 1, 2020

Alright what was this...

There were originally two instance variables isInFilemode and isSubmodule; the accessors were named alike; the variables were replaced by one called 'mode'; there is a new test method isInFileMode; in the meanwhile isInFilemode has been removed.

So the current script is broken, but not triggered because nobody loads Squot via Monticello or has GitTreeEntry instances already. (?)

Assuming this is a fresh installation, all is fine because there are no GitTreeEntry instances yet.

Assuming this is an old installation with the old inst vars, there will only be the old accessors. So these must be used.

Assuming this is a more recent installation with the new inst var, nothing should actually be done here because the migration has already happened.

So just changing the selector to the new one does not fulfill the purpose after all. What is needed is a guard to not run this conversion if it is not necessary. Both in the preamble and in the postscript. I guess this is what I meant in my earlier comment "the script could use an upgrade to be idempotent."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants