-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Feature:Added a function in gui class to support dev callback #457 #2028
base: develop
Are you sure you want to change the base?
Feature:Added a function in gui class to support dev callback #457 #2028
Conversation
…upported dataformat
@quest-bot loot #457 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @Ujj1225
Please, move the invocation of the callback
we need tests
hey, @FredLL-Avaiga |
2a38bd6
to
1111129
Compare
1fdafd4
to
080c876
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see your tests ?
efe69dc
to
ce77584
Compare
Hey i have pushed test featuring the method __get_instance of data_accessor. This is where we are actually using transformed value and getting the access type for the transformed value |
any news ? |
Are you still working on this PR @Ujj1225 ? |
yes @FredLL-Avaiga I am working on it I'll resolve the conflicts and update the branch soon! |
f42cc68
to
8eca311
Compare
hey @FredLL-Avaiga I have now swtiched to use of public api _get_data to check for the method _get_instance but still I am struggling with the use of _get_accessor()._register() |
your branch still have conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I began some test implementation
Can you check it out and finish it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there still some linter issue ?
def test_get_data_with_valid_data(gui: Gui): | ||
"""Test if get_data() returns the correct accessor for valid data.""" | ||
data_accessors = _DataAccessors(gui) | ||
data_accessors._DataAccessors__access_4_type = {int: Mock(get_data=lambda *args: "valid_data")} # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you use the _register method and the MyDataAccessor class ?
Congratulations @Ujj1225 on this merged PR |
So I have a few remarks after I ultimately got the time to look at this. Here are the reasons why I'm reluctant to merge as-is:
The bottom line is this exposed API:
It is implemented in a manner very similar to what you have proposed, and I volunteer to refactor all this. @FredLL-Avaiga what say you? |
Resolves: #457
So there was this issue where we thought of giving developer option to change unsupported data format to a supported type
For this I have made two functions in class Gui namely "on_invalid_data" and "set_on_invalid_data_callback"
The Second function "set_on_invalid_data_callback" accepts a callback as parameter and uses this function to transform data into supported format. If user doesn't provide a callback, it works as it used to. Nothing would change.
Besides in data_accessor, I have modified _InvalidDataAccessor. For all methods like get_data, get_col_types, etc. It calls the first function "on_invalid_data" to transform it into a valid data type. If no callback present it will work as it used to work.