ragardner / tksheet

Python tkinter table widget for displaying tabular data
https://pypi.org/project/tksheet/
MIT License
408 stars 50 forks source link

Modernization #176

Closed demberto closed 1 month ago

demberto commented 1 year ago

Work has begun :)

I kinda misjudged the codebase, it is huge (and too much work) to type hint it in a single go. I added all the things we discussed in #156. To use pre-commit, you will need to run pre-commit install. These changes make tksheet Python 3.7+ only, so you might wanna keep it for v7.0

All type-hint-only imports have been guarded with an if TYPE_CHECKING clause, to help the import situation. I have used several features from typing_extensions, most of them are pretty self explanatory, but if you have a question mypy's docs explain them best.

I found certain errors too. Comments beginning with # ! mark those. Those most definitely are errors.

If you are interested, I have a few more ideas:

EDIT: I am marking this as draft, as I'll need your inputs / corrections

ragardner commented 1 year ago

Thanks for all of this,

Yea, it's a lot of work lol, don't rush it. It'll take me a while to understand everything as this is all very new to me, my knowledge of coding thus far is basically just some languages and nothing else

I saw some of the bugs after using Ruff in vscode, I will have to do a release soon which just fixes those which would probably be a good place to leave tksheet before these changes, I should have been using such tools in the first place

Thanks for adding those typing conditions

I think removing the wildcard imports would be wise

I'll have to get back to you about setuptools-scm

I am unsure about CI, it sounds good but I don't know what it entails ._.

edit: Yea about some of the # ! marks, I'll fix the existing ones, some of the code is very old. I'm hoping that 6.0.5 will not be too different from this pull request at the moment to make your work easier

ragardner commented 1 year ago

Sorry about the conflicts as a result of 6.1.0 that should be the last update now before dropping python 3.6 support

o and also don’t worry about adding all the type hinting to the functions if that’s what you were suggesting, I can do that and probably get to grips with mypy at a later date to sort the remainder out

ragardner commented 1 year ago

I'm replacing the wildcard imports now so don't worry about that

demberto commented 1 year ago

Could you format with a line-length of 120, so the amount of conflicts I have to resolve just come down to what really has changed?

ragardner commented 1 year ago

Yep, sorry

I'm going to put my existing work into the new branch drop-3.6 it's unfinished so although tksheet programs should start up, selecting any cells will probably cause errors at the moment

ragardner commented 1 year ago

If it proves too different from the original you were working with I can spend some time incorporating your existing changes and then we can start from there

demberto commented 1 year ago

Alright. You let me know when you are done with it.

demberto commented 1 year ago

Some (more) ideas:

ragardner commented 1 year ago

Hey there, thanks for these

Sorry it's taking a while to get a few things done before I move on to this, it might be another couple of weeks or more still depending on if I choose to do named ranges functionality before this

I am not entirely sure what you mean by builder style methods sorry >_<

With the docstrings, do you mean putting the function explanations directly above the functions in the _tksheet.py file?

demberto commented 1 year ago

This is an example on fluent (I miswrote it as "builder") methods.

On thinking more about it though, I am a bit skeptical about having a fluent API. Optional and keyword arguments do help a lot in removing the need for them.

For docstrings, this code example pretty much sums it up.

CalJaDav commented 1 year ago

@ragardner If you'd like, feel free to delegate stuff like docstrings to me. I have enough experience with this codebase to do it.

@demberto yeah, I haven't found a huge use for fluent in python. It's hugely useful in C# (and therefore java I assume), but I think it isn't needed here. However, culling the proliferation of configuration kwargs and adding builder functions is definitely a good idea, this will bring the feel of the library to a much more professional standard. Again @ragardner, I am pretty comfortable refactoring this at some point if you're busy.

CalJaDav commented 1 year ago

I'd also like to add that I think the codebase could be made a great deal prettier with the use of decorators for handling things like event generation and sheet refreshes, etc. Again, happy to look into this at some point as it is not a functional change to the library, just a refactor.

ragardner commented 1 year ago

I am not sure about adding docstrings, especialy if it means replacing the documentation with them

Retaining the ability to permalink a particular function would mean changing the wiki or hosting it elsewhere

Writing up a lot of notes about a particular function or linking to a different function for more info would get messy and seriously bloat the file to maybe like 8000 lines

I have considered splitting the classes into different files but I'm not sure on the best way to do it or if it would be worth it

Simplifying tksheet.Sheet constructor, all the theme kwargs can be merged into a single typing.TypedDict (I already made one in tksheet.types).

I'll check it out and see if I can understand it well enough to make the changes, thanks

However, culling the proliferation of configuration kwargs and adding builder functions is definitely a good idea, this will bring the feel of the library to a much more professional standard. Again @ragardner, I am pretty comfortable refactoring this at some point if you're busy.

I'd also like to add that I think the codebase could be made a great deal prettier with the use of decorators for handling things like event generation and sheet refreshes, etc. Again, happy to look into this at some point as it is not a functional change to the library, just a refactor.

If you know how best to do this I'm certainly interested, thanks. Even just a few small changes so I can get the hang of it and do the rest would be very helpful, I struggled with understanding these topics in the brief time I spent on them

CalJaDav commented 1 year ago

Hi team, I have started refactoring, see PR #182 for some of the early stuff and some discussion around how I want to tidy up (destructively) the facade of the widget.

ragardner commented 1 year ago

@CalJaDav I've started using the walrus operator quite a bit, in comprehensions especially, I know I said don't use it when we were writing the formatting stuff sorry >_<

do you have any objections to dropping compatibility for Python 3.7 and going to 3.8

ragardner commented 1 year ago

regarding the span class and arguments such as reset col positions - don't worry about any of that just keep it very simple because I think when it comes to overwriting the sheet data completely there will have to be a dedicated separate function anyway

Also don't worry about getting it to work with everything, if it's very clunky then just for now we'll make it so functions like create_dropdown can take a span class as an argument, and I think with just a small portion of code for an example I'll be able to do the rest

At the moment I am working on making some external functions (functions found in Sheet) utilize the internal functions, such as inserting rows/columns using an internal function which is also used by end user insert rows/columns events. This means that the developer will also have access to the undo stack with calls to insert/delete etc

I also will break down some things inside MainTable into various sheet operations such as clear, delete, add, move, set data which will be used by both functions in Sheet and MainTable

I've basically finished:

I will have to adjust some things again for the changes I listed above however

The current state of the library will be in drop-3.6, there may be some bugs but hopefully not too many, ignore the existing span class that was just me messing around

And @demberto I thought some more about what you mentioned about adding in a treeview, I really appreciate the cautionary thought about designing the api. I have an idea in my mind about how the treeview will work and the api it will use and I think it will be okay but 🤷‍♂️ I think I'll have to cross that bridge if I come to it

ragardner commented 1 year ago

An update, I have managed to get syntax such as self.sheet.span("A1").value working by storing the sheet widget reference in the span dict and overriding the span dicts __getattr__ and __getitem__ methods to point at the sheet widgets __getitem__ method if looking for value or data

I am not sure if this is a good way to do it or not but I wanted to try nonetheless

It can expand both ways by using a slice e.g. "A1:"

Or one way, right - "A1:2", down - "A1:C"

I could perhaps add an expand arg which achieves the same

The span dict can store options such as index and displayed as well which I'll make use of

Will continue working on this

CalJaDav commented 1 year ago

Hey mate! Sorry for going AWOL, ski season is starting up on my hemisphere and I have been trying to push as many work projects out the door before I take a few weeks off to play in the snow. I'll take a look at your changes this weekend hopefully :)

ragardner commented 1 year ago

Hey, no worries at all, no need to look things over if you're real busy it's a work in progress anyways

I managed to get the xlwings style syntax working pretty flawlessly it seems, thanks again for the suggestion I'm really happy with it

I've got some stuff to finish and polish but at the moment for the code on branch drop 3.6 you can do stuff like

# getting formatted transposed data, header included
self.sheet["A1"].expand().options(format=int_formatter(nullable=False), header=True, index=False, transpose=True).data

# setting data with prior format + add to end user undo stack, header, index also allowed
self.sheet["A1"].options(format=float_formatter(), undo=True).data = [[f"{r}{c}" for c in range(9)] for r in range(9)]

str keys use excels indexing, slice keys use pythons indexing

No documentation yet though

ragardner commented 1 month ago

Hello, I hope you're both doing well

I am now sort of wrapping up this project as I must move on and I have added a thank you section to the docs naming you both, if that's okay, it's here: https://github.com/ragardner/tksheet/wiki/Version-7#contributors-and-special-thanks

I will still be bug fixing etc if need be

Cheers and safe travels

CalJaDav commented 1 month ago

Thanks mate! It was a pleasure working with you, this is a very good little library you have put together here. You should be proud.