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

Modernization #176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Modernization #176

wants to merge 1 commit into from

Conversation

demberto
Copy link

@demberto demberto commented May 4, 2023

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:

  • Eliminating wildcard imports entirely.
  • Using setuptools-scm for version management.
  • Adding some sort of CI (just basic code-quality checks, for now).

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

@demberto demberto marked this pull request as draft May 4, 2023 16:16
@ragardner
Copy link
Owner

ragardner commented May 4, 2023

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
Copy link
Owner

ragardner commented May 4, 2023

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 ragardner marked this pull request as ready for review May 4, 2023 20:06
@ragardner
Copy link
Owner

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

@demberto
Copy link
Author

demberto commented May 5, 2023

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
Copy link
Owner

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
Copy link
Owner

ragardner commented May 5, 2023

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
Copy link
Author

demberto commented May 5, 2023

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

@demberto
Copy link
Author

Some (more) ideas:

  • Simplifying tksheet.Sheet constructor, all the theme kwargs can be merged into a single typing.TypedDict (I already made one in tksheet.types). Some of the other options which don't actually affect the class initialisation and are just stored into instance attributes for later use, can be moved off into builder-style methods.
  • Docstrings, which can easily replace many things in DOCUMENTATION.md and be much more clear and accessible.

@ragardner
Copy link
Owner

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
Copy link
Author

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
Copy link
Contributor

CalJaDav commented May 18, 2023

@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
Copy link
Contributor

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
Copy link
Owner

ragardner commented May 19, 2023

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 CalJaDav mentioned this pull request May 21, 2023
@CalJaDav
Copy link
Contributor

CalJaDav commented May 23, 2023

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
Copy link
Owner

@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
Copy link
Owner

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:

  • Moving non consecutive rows/columns
  • Deleting non consecutive rows/columns
  • Internals for named spans for all major sheet operations
  • undo/redo for above

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
Copy link
Owner

ragardner commented Jun 12, 2023

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
Copy link
Contributor

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
Copy link
Owner

ragardner commented Jun 16, 2023

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

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.

None yet

3 participants