Merge data explorer into the main refactor 24 branch. - #4134
Conversation
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
87d6e3d to
ca424bd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
DrPaulSharp
left a comment
DrPaulSharp
left a comment
There was a problem hiding this comment.
All looks good. I've a couple of points to consider:
- It may be time to consider improving some temporary names in this branch. I'm thinking of the files
refactored.py,refactored_data_explorer.pyand the classesNewDataExplorerandNewDataManager. The time to rename them might not yet be right, but worth thinking about at this point. - It may be worth considering how this branch, and merging it, will impact on other branches submitted recently. The example that made me think of this is the use of
temp_xml_readerinMetadataExplorer, which is renamed in SasView/sasdata#235.
| self.filesWidget = DataExplorerWindow(self._parent, self, manager=self._data_manager) | ||
| # self.filesWidget = DataExplorerWindow(self._parent, self, manager=self._data_manager) | ||
| # TODO: Is this a good opportunity to change this name? dataExplorer would be better I think. | ||
| self.filesWidget = NewDataExplorer(self._data_manager ,self._parent) |
There was a problem hiding this comment.
Good question. Maybe consider this again now before merging this branch in.
There was a problem hiding this comment.
I'm a bit worried about potential merge conflicts if this renamed before other work is merged. I suggest that I create an issue after merging this branch, get other things merged into the base refactor branch, and then make a separate branch to rename this.
Yes that is true. My suggestion is that I make a new issue regarding this, and we move it when we feel it is appropriate. That work would also involve removing the previous data explorer. Tbh, I think we are coming closer to the point where this should be done so I might suggest we do this sooner rather than later.
My plan is to make the necessary changes once Jeff has completed all of his work on importers. There will be quite a few things that need to be changed, so I'm just waiting for it all to be ready so I can do it all in one go. |
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
2 participants
As discussed in the last meeting, to keep the branches more organised, I am requesting to merge data explorer into the main refactor branch so that the refactor branch can be the main point for merges.
This brings all of the work in data explorer (including ascii reader, data loader interaction with sasdata etc) into the main refactor branch.