Closed CalJaDav closed 1 year ago
I think something needed to be changed yea, but I changed the arg to get_displayed
but should a user provide return_copy
as a keyword arg it will not produce an error, as a positional arg it will make the data getters return the displayed values rather than the formatter data
I will review the docs and contributing soon, thanks
I have decided to change the default formatter, everything still works with the original formatters but with some changes to argument and function names, you can find both in tksheet/_tksheet_formatters.py and some tests with the new formatter in test formatters 1 and the existing ones in test formatters 2 in the tests folder
The rationale for this change is that I felt it would be easier for users to provide a datatypes
, format_func
and a to_str_func
than a class and I wanted this option anyway
The new Formatter()
takes extra kwargs as well and passes them to those functions and also a clipboard function if the user has provided one (if not then there is a default clipboard function)
Minus some changes to your original classes, like adding a clipboard()
function and changing some variable and function names everything should still work as before
Let me know if you have any thoughts or issues with these changes
Really good updates. I definitely prefer your implementation of formatters. While writing the docs I have renamed a few things just because I think it makes more sense given how the users is going to interact with formatters from now on.
I will add a section in about creating your own formatter classes, as this is something I see being useful for users with very specific crtieria (like some of the weird datatypes my tksheet-based CRUD interface will deal with), but custom formatter classes will just inherit from the base Formatter
class. This is not the recommended way for most users to do this obviously, the user should just use the formatter function interfaces we provide, but I do foresee a scenario where someone might want a cell to do a very specific thing, so I think it is prudent to retain it as an option.
Removed:
Minor changes:
set_cell_format(formatter
-> formatter_class
. Most users will not need to alter the class itself, I think this makes more sense.set_cell_format(formatter_kwargs
-> formatter_options
. I think this is more readable.dtype_formatter_kwargs
-> dtype_formatter
. These are interfaces are the "formatters" now for (most) users so I think the name needed to reflect this. formatter
function. This is just a blank template for users to fill in with their own stuff, but it makes sure users fill in all the important bits.int_to_str
-> to_str
. This is the most basic string formatter and so now is the default.Material Code Changes and Bug Fixes:
__eq__
method to the Formatter class. This to fix a persistent undo bug where opening but not changing a formatted cell would always cause an entry to be added to the undo stack. This is because comparing the cell value (a formatter class) to the text editor output (a str) would always result in an inequity. The custom __eq__
method addresses this. Suggested Code Changes:
formatter.value
should be added to the undo stack, not the foramatter.__str__()
. Currently, if I delete a Bool cell containing a 7
(displays "NA") and I undo it, the cell will now have formatter.value
= "NA"
not 7
. The intention with storing a polymorphic value parameter separate to the string or datatype representation is so that users can retrieve and "have another go" if they screw up entering a parseable input into a formatted cell on the first try. This should be a relatively straightforward change, I suggest either storing formatter.value
or f"{formatter.value}"
on the undo stack instead of f"{formatter}"
, but I'll leave the precise implementation to your discretion. The above changes and thoughts are great and very helpful thank you
Don't worry about working on this just for the next few days, I don't want you to write a lot of stuff for the current implementation only for me to approve it and then change my mind about it later and overwrite it
I am going to try out a few solutions for both above suggested changes that may involve changing code in other areas
it may even involve storing the actual data (e.g. a datetime object) in the table instead of a formatter class (I'm not sure about this just yet though). Perhaps there would be the loss of some customisation in the class functions if I can’t make it work with a custom class as it does now… all of the other functionality would be retained, the cell options would act as format_options storage
edit: I can probably make these changes without removing the ability to have a formatter class
I am sorry have to repeatedly changed my mind on the implementation of formatters, I think this latest implementation is the final one though...
Basically by default instead of storing a Formatter()
class in the table data the actual values are stored there, though there is still the possibility to use a class
(I think?) this should deal with the above issues of comparisons, dunder methods etc. I think that the latest version has also solved the problem with deleting a cell containing an invalid value, as far as I can see
I have updated the docs to reflect the change, thanks again for doing all that
So yea I'm sorry for the spam and stuff, hopefully cell formatting is near completion now
Great! Thanks again for all the hard work. I agree with all your changes, storing actual values on MT.data definitely makes more sense. I'll play around with it today and see if I can catch any bugs, but otherwise I think this branch is almost ready to be merged back to master.
Updated the docs and removed the
get_formats
argument from the get_data functions. Now there is only thereturn_copy
argument, as it was before the update. If thereturn_copy
is true, then it returns a copy of the data as it is displayed on the table, i.e. all cells are parsed throughf"{c}"
. Otherwise, it will return the data in its converted datatype. Raw data can be accessed throughMT.data
, which will include the cell formatters. I have addressed this in the docs and there is an updated example in section 32.Let me know if you think this not the correct behavior.
Cheers, C