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

Altered annotations are removed #58

Closed
Firesphere opened this issue Apr 15, 2016 · 14 comments
Closed

Altered annotations are removed #58

Firesphere opened this issue Apr 15, 2016 · 14 comments

Comments

@Firesphere
Copy link
Member

When an annotation is edited by the user, it gets destroyed.
It would be nice to compare the existing, and only destroy those that are not needed anymore.

@axyr
Copy link
Contributor

axyr commented Apr 18, 2016

i dont think thats gonna work..

Hoe do we know if a tag is altered by dev or the orm/extension spec is changed?

@Firesphere
Copy link
Member Author

I would think, a basic compare, Run on all supported tags, see if the tag differs greatly (e.g. added explanation).

I don't have a solution or anything for it, but the current volatile method of just destroying whatever is found, should be more dev-friendly I'd think.

@axyr
Copy link
Contributor

axyr commented Apr 18, 2016

You have an example?

@Firesphere
Copy link
Member Author

Of destroyed additions? That's easy to reproduce.

Or you mean how I'd solve it?
In that case... no, I've been pondering how to solve it. I'm thinking something of an array-like method, but I'm currently more focused on refactoring Security.
I'll have a talk with the guys here if they have an idea.

@assertchris
Copy link

assertchris commented Apr 18, 2016

You could account for changes by:

first run

  1. Cache properties
  2. Cache annotated state (after your script has added annotations)

subsequent runs

  1. Create new annotated state (run your script)
  2. Create old annotated state, from old cached properties
  3. Create a diff between old annotated state and current un-annotated state
  4. Apply the diff over the new annotated state

Edit: here is a diff library which appears to have the required granularity: http://www.raymondhill.net/finediff/viewdiff-ex.php

@Firesphere
Copy link
Member Author

But that would take all annotations into account right? Therefore, still destroy if the new version has them?

@assertchris
Copy link

Huh? I don't understand what you're saying.

If you started with:

class Foo extends DataObject {
    private static $db = [
        "Bar" => "Varchar",
    ];
}

...then run your script, resulting in:

/**
 * @property string $Bar
 */
class Foo extends DataObject {
    private static $db = [
        "Bar" => "Varchar",
    ];
}

Say the user changed it so:

/**
 * @property string $Bar This property holds such and such...
 */
class Foo extends DataObject {
    private static $db = [
        "Bar" => "Varchar",
    ];
}

You could re-generate the annotated version (from a cached property set), and compare that to what exists in the file. You'd get something like "user versions has extra string at line and column.."

So, you generate an annotated version from the new property set and apply that required change on top of it...

@Firesphere
Copy link
Member Author

The problem occuring, is removed properties ;) That should be removed, altered or not, since the property doesn't exist anymore.

@assertchris
Copy link

Again, I think the answer is to store a copy of the properties each time you run the script and compare to know what should be generated differently vs. what the user has changed.

@Firesphere
Copy link
Member Author

Sorry, I'm quite tired. I misunderstood what you meant.
I'll re-think my thoughts when I'm not this tired. And would like to have a quick chat next hackday about this :)

@axyr
Copy link
Contributor

axyr commented Apr 18, 2016

I was more thinking on when "Bar" => "Varchar" becomes "Bar" => "Int" or even Bar() // has_one

You are right if we can get a copy of the properties before the DB is altered we could make a comparison.

I will take look at it.

@madmatt
Copy link

madmatt commented Apr 18, 2016

The only thing you can expect to change is the description, right? This could be solved by preserving the description text if it exists, and something in the README along the lines of:

This module will remove any annotations that are no longer required, including any documentation you have written. We suggest using a version control system such as Git, and checking all changes thoroughly.

I'm not sure it needs to have a cache of what happened the last run, either you want this to auto-document your classes or you don't, so the 'cache' should just be 'read the existing docblock, remove what is no longer needed based on the current class, and copy any old descriptions across'. Thoughts?

@axyr
Copy link
Contributor

axyr commented Apr 18, 2016

DBField type can change as well, while the comment should remain..

@axyr
Copy link
Contributor

axyr commented Apr 19, 2016

I think this should do it :

ecef641

@axyr axyr closed this as completed in ecef641 Apr 19, 2016
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

No branches or pull requests

4 participants