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

Sweep.py: add getters and setters for private fields #659

Merged
merged 3 commits into from
Jul 30, 2023

Conversation

asarhaddon
Copy link
Contributor

The two first commits are trivial (and could be submitted separately if needed).
The third one is a proof of concept for one of the ideas in issue #657.
The header describes why it can probably not be applied yet.

  • Refactoring (no functional changes, no API changes)
    No breaking change (hopefully).

Beware that this commit removes a lock from
SweepSettings.update_tex_power, and adds one to
DeviceSettings.updatecustomPoint.
Both changse may be incorrect, depending on the role of the lock
(issue #657).

Follows: 6eb24f2 d09b55e dbea311

Since d09b55e, the Properties.name class attribute is overriden by
each assignment to the properties.name instance attribute.
This is most probably unwanted.

This commit

 * removes @DataClass, which is confusing as some attributes are
   managed because of the lock.
   Because of this, it has to restore __repr__ and __eq__.
 * provides getters and setters for private attributes, and
   protects each update by a thread lock
 * adds a regression test for the bug fixed by d09b55e (immutable
   properties).
@asarhaddon asarhaddon requested a review from zarath as a code owner July 16, 2023 19:42
Copy link
Collaborator

@zarath zarath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanx for the changes

@zarath zarath merged commit abb80a5 into NanoVNA-Saver:main Jul 30, 2023
@asarhaddon asarhaddon deleted the sweep-setters branch August 15, 2023 20:06
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