-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: master
Are you sure you want to change the base?
Modernization #176
Conversation
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 Thanks for adding those typing conditions I think removing the wildcard imports would be wise I'll have to get back to you about I am unsure about CI, it sounds good but I don't know what it entails ._. edit: Yea about some of the |
Sorry about the conflicts as a result of 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 |
I'm replacing the wildcard imports now so don't worry about that |
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? |
Yep, sorry I'm going to put my existing work into the new branch |
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 |
Alright. You let me know when you are done with it. |
Some (more) ideas:
|
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 |
@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. |
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. |
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
I'll check it out and see if I can understand it well enough to make the changes, thanks
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 |
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. |
@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 |
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 At the moment I am working on making some external functions (functions found in I also will break down some things inside 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 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 |
An update, I have managed to get syntax such as 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. Or one way, right - 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 |
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 :) |
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 |
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:
setuptools-scm
for version management.EDIT: I am marking this as draft, as I'll need your inputs / corrections