-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Add an Object Explorer to the Variable Explorer #8852
Conversation
@dalthviz, before you proceed further, please break |
02ece12
to
31db31d
Compare
@dalthviz, this is looking nice, thanks! Jus another note so you keep working on this: I noticed some of the new files you added are using the Please remove that dependency and instead replace calls to |
Overall, good job handling incorporating the external project, aside from a few minor issues I point out in my review. Additionally, one more thing: Three different headers types are used in three different sets of files:
Particularly for new files, we should use one consistent form and stick with it. The canonical form I've been using in new files going forward is:
(Picking the appropriate bracketed verbiage for the use case). Unless the original file has explicit license-related text in its headers that is substantially different from just a statement that the file is released under the LICENSENAME (which if so, should be preserved) there is no need to repeat the same information (distributed under the MIT License) twice. Any other information should be included below the "see LICENSE.txt..." line separated by one blank line, so the license statement is clean and readable and there is no possibility of missing or misreading it. In this case since this is included as a self-contained directory, you should include the original
|
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.
Thanks for adding this, @dalthviz ! I tested it and I'm not sure if you already plan to fix these issues or not, but I figured I'd briefly mention them:
- Regression: The
Object
should be expanded one level and callable/special attributes hidden by default, so they can be easily spotted like in the previous version of the Object Explorer - Regression:
Pretty print
should be selected by default, notpath
, since the user is far more likely to be interested in the former than the latter - Major Regression: Cannot edit or open a viewer/editor for collections, values, arrays, dataframes etc. that are attributes of objects, like I could before
- UI: Colors need to be fixed to work with the dark theme
- UI: Help -> About menu is not necessary and useless as is (as the link is not clickable or even selectable) and should be removed;
- UX: Settings (in View menu) should be preserved, i.e. new dialogs should launch with the same settings as the last modified dialog; its incredibly tedious to set them every time
- UI: Column widths should be much narrower (and no wider than the data within), with
unicode
removed by default as its mostly redundant andtype
placed second aftername
- UI/UX: "Refresh", "Auto Refresh", "Show callables" and "Show Special" should all be toolbar items, not view mention options, for much easier/quicker access, and make "Table Columns" a top-level menu
- UX: You should add an option to the
NamespaceBrowser
context menu to open the object explorer for objects, since the user sometimes might want to view all the extra properties and such of things we open bespoke editors for by default
Also, is there any way to avoid the objecteditor
/objecteditor2
/objecteditor2.objecteditor
situation? It looks a bit like "code smell" and could be confusing, IMO, to someone newer to the codebase. For example, you could rename objecteditor2
and objecteditor
to objectexplorer
(to match the official name of the feature, and its functionality since it doesn't actually allow editing yet and is different from all the other "editors").
spyder/plugins/variableexplorer/widgets/objecteditor2/tests/__init__.py
Outdated
Show resolved
Hide resolved
@CAM-Gerlach thank you for testing this and the detailed list of missing elements! I will take them as a TODO list 👍 |
Thanks @dalthviz ! And if any of them are determined not to be in scope here, I can make them a separate (meta)-issue instead. |
c93a870
to
89058ea
Compare
This is looking really good, thanks @dalthviz! My comments:
These are a lot of tasks, but fortunately almost all of them are small. |
@ccordoba12 also spyder-ide/spyder-kernels#100 needs to be merge for this to be done |
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.
@dalthviz, I left some minor comments about the code, this is almost ready!
spyder/plugins/variableexplorer/widgets/objectexplorer/utils.py
Outdated
Show resolved
Hide resolved
spyder/plugins/variableexplorer/widgets/objectexplorer/utils.py
Outdated
Show resolved
Hide resolved
Some last comments about the organization of the widget:
|
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.
Thanks a lot @dalthviz for your hard work on this one! It's a fantastic addition to our variable explorer and something I'm sure many of our users will really appreciate.
I only made some minor adjustments and fixed our tests to make them pass.
@dalthviz, please don't forget to open an issue about this:
so you don't forget to address it in beta4. |
Description of Changes
Issue(s) Resolved
Fixes #558
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct: dalthviz