Add error handling for extract of incomplete block

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉

[raymyers]

Description

Raise exception when extracting the start of a block without the end, examples in the issue and the included tests.

Fixes#734

The added logic is:

• Raise when:
  • region end is more indented than region start, AND
  • the next line after region end is indented as much as region end or more.
• When any of these (start, end, or next) are on a blank/comment line, consider the next "real" line instead.

This seems correct to me and passes existing tests, but it could use a look by someone familiar with this part of the code to make sure it makes sense and is following conventions.

Checklist (delete if not relevant):

• I have added tests that prove my fix is effective or that my feature works
• I have updated CHANGELOG.md

[lieryan]

Thanks for making this PR, @raymyers. IIUC, the current implementation here might not correctly handle multiline strings that have text indented less than the code or function calls/dicts/arguments that uses certain syntactically valid, but non-standard indentations, for example:

def foo():
if foo:
s = """
blah blah
blah
"""
print(
a, b, c
)

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils, exctract is only intended to work on source code that is syntactically valid python (i.e. they must be ast-parseable). What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block. That would allow the user to be a bit lazy and just select a few lines at the top of the block when extracting a block of statement.

Do you want to take a stab at reimplementing this with ast?

[raymyers]

Thanks for reviewing, @lieryan.

I suspect that it might be easier to handle all the edge cases for this if you're checking with the ast instead of sourceutils. [...] What you want to do is check that except at the top level of the selected AST node, the children node are either completely covered by the selected text range or not at all.

Good idea to use AST, I've updated it using the logic "there should be no node starting within extract range that ends after the range". Please take another look when you get a chance.

Another interesting alternative behavior could be that instead of producing an error, extract could have just automatically expanded the selection to cover the whole body if the user selected a partial block.

As a new contributor of course I don't speak for the direction of Rope, but I'll explain my rationale for suggesting this check be strict and erroring out rather than expanding:

I suspect it's not always going to be clear what direction was intended to change the selection (e.g. expand bottom vs shrink top). If anyone wants to try and repair the selection, maybe that should happen in the calling UI rather than here. Probably the calling app shouldn't have let a malformed selection get this far, failing to raise an error could mask that problem as it did in the usecase that led me to submit this. Intellij also disallows these invalid ranges fwiw.

[lieryan]

Thanks for the contribution @raymyers, this looks great to me.

@all-contributors add @raymyers for code

[allcontributors]

@lieryan

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @raymyers! 🎉