Skip to content

Update method fix - #85

Open
cduff4464 wants to merge 4 commits into
Nikea:masterfrom
cduff4464:updateMethodFix
Open

Update method fix#85
cduff4464 wants to merge 4 commits into
Nikea:masterfrom
cduff4464:updateMethodFix

Conversation

@cduff4464

Copy link
Copy Markdown

As was brought up in the issue I created, there is currently no method for the CrossSectionMainWindow class. This pull request simply creates a method for this class to add new data to the cross section viewer.

@CJ-Wright

Copy link
Copy Markdown
Contributor

FYI you can reference a PR or issue by using the # sign. For example the issue mentioned is #84


self.key_list = key_list

def add_data(self, new_data, new_key_names):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc strings in this would be appreciated. Please use the numpy doc string format.

@CJ-Wright

Copy link
Copy Markdown
Contributor

Have you not written tests for this addition?

@CJ-Wright

Copy link
Copy Markdown
Contributor

Also it seems that the travis.yml file has some issues @tacaswell @danielballan have you seen these errors before?

@tacaswell

Copy link
Copy Markdown
Member

👍 , mostly agree with @CJ-Wright 's comments. The classes in this code are a bit out of control, I would not do them this way again if starting from scratch.

It looks like much of this logic should fall back to the Messenger class and the code at this level. should mostly delegate (you should not be reaching in to do self._messenger._ctrl_widget._spin_img.setMaximum(len(self.key_list)-1) as that is going across 3 layers of things telling you they are 'private').

@tacaswell

Copy link
Copy Markdown
Member

to fix the travis issue, change the channel from scikit-xray -> scikit-beam (test that locally to make sure I remembered the name right).

@tacaswell

Copy link
Copy Markdown
Member

And if doing the messenger stuff 'right' turns into a complete hairball, I would lean on the side of just making this work.

@mrakitin

Copy link
Copy Markdown
Member

What's the status of this PR?

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.

4 participants