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

Two more ways to change output method temporarily #220

Merged
merged 4 commits into from Feb 11, 2017

Conversation

Projects
None yet
2 participants
@plq
Contributor

plq commented Jan 5, 2017

Another way of changing output method temporarily.

@plq plq changed the title from Implement IncrementalWriter.method() to Add method keyword to _IncrementalWriter.element() Jan 5, 2017

@plq plq changed the title from Add method keyword to _IncrementalWriter.element() to Two more ways to change output method temporarily Jan 6, 2017

Show outdated Hide outdated src/lxml/serializer.pxi
def __cinit__(self, _IncrementalFileWriter writer not None, int method):
self._writer = writer
self._new_method = method
self._old_method = writer._method

This comment has been minimized.

@scoder

scoder Jan 8, 2017

Member

It's not correct to remember the original value here. Context manager objects can be reused and there is no guarantee that users create this object directly as part of a with-statement, so the exit method might store back an incorrect value.
Either reject an unexpected current method value when entering/exiting to prevent reuse, or use a stack for storing away the old values. Preventing reuse seems simple and acceptable, though (and would obviously need testing :-) ).

@scoder

scoder Jan 8, 2017

Member

It's not correct to remember the original value here. Context manager objects can be reused and there is no guarantee that users create this object directly as part of a with-statement, so the exit method might store back an incorrect value.
Either reject an unexpected current method value when entering/exiting to prevent reuse, or use a stack for storing away the old values. Preventing reuse seems simple and acceptable, though (and would obviously need testing :-) ).

This comment has been minimized.

@plq

plq Jan 8, 2017

Contributor

I guess I'm among the guiltiest when it comes to using context managers outside of with statement. Look for __enter__ and __exit__ cals in: https://github.com/arskom/spyne/blob/f92823a512eb8be71adb24fa151b397f27df5b9f/spyne/protocol/cloth/to_cloth.py#L278

To me the contract with using context managers' private apis is it's user's responsibility to enter/exit in correct order. I could totally take the precautions you mention but aren't they needless overhead for people who use the apis the correct way (within the with statement) or incorrect way (outside the with statement) but who are supposed to be people who know what they are doing?

Maybe in the spirit of protecting the expert, we could set a flag on the context manager to prevent it from being __exit__'ed twice.

However if you insist, I will implement the stack.

@plq

plq Jan 8, 2017

Contributor

I guess I'm among the guiltiest when it comes to using context managers outside of with statement. Look for __enter__ and __exit__ cals in: https://github.com/arskom/spyne/blob/f92823a512eb8be71adb24fa151b397f27df5b9f/spyne/protocol/cloth/to_cloth.py#L278

To me the contract with using context managers' private apis is it's user's responsibility to enter/exit in correct order. I could totally take the precautions you mention but aren't they needless overhead for people who use the apis the correct way (within the with statement) or incorrect way (outside the with statement) but who are supposed to be people who know what they are doing?

Maybe in the spirit of protecting the expert, we could set a flag on the context manager to prevent it from being __exit__'ed twice.

However if you insist, I will implement the stack.

This comment has been minimized.

@scoder

scoder Jan 8, 2017

Member

I actually don't think that we need a stack here. It should be enough to check that the output method didn't change between the end of __enter()__ and the start of __exit()__, i.e. to save the old method on entry and check that the current method is still new_method on exit. If it's not, it's a bug in the calling code and the user should know about it, i.e. get an exception. Oh, and maybe prevent re-entry before exiting by checking that old_method is set to an invalid value on entry. That would detect programming problems while not restricting users too much.

@scoder

scoder Jan 8, 2017

Member

I actually don't think that we need a stack here. It should be enough to check that the output method didn't change between the end of __enter()__ and the start of __exit()__, i.e. to save the old method on entry and check that the current method is still new_method on exit. If it's not, it's a bug in the calling code and the user should know about it, i.e. get an exception. Oh, and maybe prevent re-entry before exiting by checking that old_method is set to an invalid value on entry. That would detect programming problems while not restricting users too much.

This comment has been minimized.

@plq

plq Jan 10, 2017

Contributor

It should be enough to check that the output method didn't change between the end of enter() and the start of exit()

done.

Oh, and maybe prevent re-entry before exiting by checking that old_method is set to an invalid value on entry.

not like that, but done.

@plq

plq Jan 10, 2017

Contributor

It should be enough to check that the output method didn't change between the end of enter() and the start of exit()

done.

Oh, and maybe prevent re-entry before exiting by checking that old_method is set to an invalid value on entry.

not like that, but done.

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Jan 17, 2017

Contributor

Please let me know if you need anything else

Contributor

plq commented Jan 17, 2017

Please let me know if you need anything else

@scoder

scoder approved these changes Feb 11, 2017

@scoder scoder merged commit 4939666 into lxml:master Feb 11, 2017

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Feb 13, 2017

Contributor

thanks a lot!

Contributor

plq commented Feb 13, 2017

thanks a lot!

plq added a commit to plq/neurons that referenced this pull request Feb 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment