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

Fixed the error while setting tim drectory for excised tim file. #82

Closed
wants to merge 1 commit into from

Conversation

lanky441
Copy link

@lanky441 lanky441 commented Oct 2, 2024

As tim_directory is now a property of TimingConfiguration class with the setter function set_tim_directory, it was giving an error while setting the tim directory for excised tim file using self.tim_directory = ''. I have now changed it to self.set_tim_directory = '' to fix that.

@lanky441 lanky441 requested a review from rossjjennings October 2, 2024 21:05
@rossjjennings
Copy link
Member

@lanky441 I introduced this bug in #63 by misunderstanding how setter functions work and failing to adequately test my code. I essentially wrote this:

class TimingConfiguration:
    @property
    def tim_directory(self):
        # get the tim directory
    @tim_directory.setter
    def set_tim_directory(self):
        # set the tim directory

thinking that the name of the set_tim_directory() function didn't matter, since the name of the Property was already set by the tim_directory() function. But in fact, the name of the setter does matter, and should have been tim_directory() for it to work in the usual way, where there is only one property tim_directory with both a getter and a setter, which is what I intended.

What the above code actually does is create two properties, one called tim_directory with only a getter function, and a second one called set_tim_directory with the exact same getter function, but an additional setter function. That's why getting the value with tc.tim_directory works fine, but to set it you have to do tc.set_tim_directory = ''. This is a byproduct of the way that property decorators work, where each one creates a new Property object that replaces the last one.

@rossjjennings
Copy link
Member

The correct way to fix this is at the source, by changing the name of the setter to tim_directory() instead of set_tim_directory(). There's a couple of similar changes that I should make, too. I'll make a separate PR to do that, and close this one.

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