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

Bug in development.refactor.move_class when updating class path #382

Closed
AntoineGautier opened this issue Oct 30, 2020 · 0 comments · Fixed by #383
Closed

Bug in development.refactor.move_class when updating class path #382

AntoineGautier opened this issue Oct 30, 2020 · 0 comments · Fixed by #383
Assignees

Comments

@AntoineGautier
Copy link
Contributor

AntoineGautier commented Oct 30, 2020

The function _getShortName converts the class reference A.B.C.X.Y in the file A/B/C/D/E.mo into X.Y.
This is incorrect in the case where a subpackage X also exists under D (typically BaseClasses).
In that case the function should return C.X.Y.
However, the class reference A.B.C.D.X.Y should indeed be converted into X.Y.
So the logic is as follows.

  1. Find the first element of the path that differs between the class and the file (X in the above example).

  2. Test if a class with that name (X) exists at any level below, in reference to the file path (under A/B/C/D/. in the above example).

    • false: the short class name should start with that element (X).
    • true: the short class name should start with the prior element (C).

In addition, in the function _updateFile

        if "." in shortSource:
            replace_text_in_file(srcFil, shortSource, shortTarget, isRegExp=False)
        else:
            regExp = r"(?!\w)" + shortTarget
            replace_text_in_file(srcFil, regExp, shortTarget, isRegExp=True)

regExp = r"(?!\w)" + shortTarget should be regExp = r"(?!\w)" + shortSource.

EDIT: Also the negative lookahead assertion (?!\w) has no effect if not preceded with ^ for instance. So

re.sub(r'(?!\w) Class', ' Test', 'model Class')

returns model Test which is the undesired behavior. Better use the negative lookbehind assertion (?<!\w).

@AntoineGautier AntoineGautier self-assigned this Oct 30, 2020
@AntoineGautier AntoineGautier changed the title development.refactor.move_class fails to update type specifier preceded by = sign development.refactor.move_class fails to update type specifiers Oct 30, 2020
@AntoineGautier AntoineGautier changed the title development.refactor.move_class fails to update type specifiers Bug in development.refactor.move_class when updating class path Oct 30, 2020
AntoineGautier added a commit that referenced this issue Oct 30, 2020
mwetter pushed a commit that referenced this issue Nov 10, 2020
* Fix bugs from #382

* Run autopep8

* Use negative lookbehind assertion

* Improve _getShortName and avoid writing file if no change

* Minor

* Remove debug log

* Run autopep8

* Locate _getShortName at top level so it can be tested + guard against idx_start < 0

* Add test for _getShortName

* Fix root path definition

* Run autopep8
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 a pull request may close this issue.

1 participant