Skip to content

Remove all usage of python2_3.py#1939

Merged
j9ac9k merged 8 commits intopyqtgraph:masterfrom
ksunden:rm_python2_3
Aug 2, 2021
Merged

Remove all usage of python2_3.py#1939
j9ac9k merged 8 commits intopyqtgraph:masterfrom
ksunden:rm_python2_3

Conversation

@ksunden
Copy link
Copy Markdown
Contributor

@ksunden ksunden commented Jul 31, 2021

Technically these functions were exported at the top level of the library, this removes them without warning... If we want to we can bring them back for there and add deprecation warnigns, but I honestly don't think its needed, as we are py3 only now and have been for multiple releases.

This may introduce a number of 'useless cast' or similar but those were always happening anyway

This PR brought to you by sed

Technically these functions were exported at the top level of the library, this removes them without warning... If we want to we can bring them back for there, but I honestly don't think its needed, as we are py3 only now and have been for multiple releases.

This may introduce a number of 'useless cast' or similar but those were always happening anyway

This PR brought to you by sed
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Aug 1, 2021

Nice!

On mobile so only skimmed a small portion of the diff. Saw some cases assigned range = ... which we probably shouldn't assign Python keywords to variables

@ksunden
Copy link
Copy Markdown
Contributor Author

ksunden commented Aug 1, 2021

literally all I did was sed -i s/asUnicode/str/g and similar , not surprised there are some unnecessary casts...

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Aug 1, 2021

my only comment is regarding the unnecessary casts... besides that, this LGTM.

if self._ignoreIndexChange:
return
self._chosenText = asUnicode(self.currentText())
self._chosenText = str(self.currentText())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

QComboBox::currentText() can remove str casting

pass
if data is None:
return asUnicode(self.itemText(ind))
return str(self.itemText(ind))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

QComboBox::itemText(index) returns a string as well, so we can remove the casting here too


for c in columns:
row.append(asUnicode(self.horizontalHeaderItem(c).text()))
row.append(str(self.horizontalHeaderItem(c).text()))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

QTableWidgetIem::text returns a string, so we can ditch the casting here as well


assert sb.value() == value
assert pg.asUnicode(sb.text()) == expected_text
assert str(sb.text()) == expected_text
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

QSpinBox::text returns str type as well.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Aug 2, 2021

Alright, I think I'm done 😬 had to do a lot of doc lookups for the Qt stuff.... so thankful for Dash docs letting me lookup the stuff insta-fast.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Aug 2, 2021

@ksunden thanks for the PR, this LGTM, merging!

@j9ac9k j9ac9k merged commit a472f8c into pyqtgraph:master Aug 2, 2021
@ksunden ksunden deleted the rm_python2_3 branch August 2, 2021 04:43
@j9ac9k j9ac9k mentioned this pull request Aug 2, 2021
w.value = lambda: asUnicode(w.text())
w.setValue = lambda v: w.setText(asUnicode(v))
w.value = w.text
w.setValue = w.setText
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.

@j9ac9k I'm almost inclined to leave setValue as casting to string. While you can never get a non-string from text(), you can certainly set with e.g. an int and this would safely convert. What are your thoughts? It plays into #1919 merging.

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.

The new definition of _interpretValue() in 1919 should handle a call to setValue(10), but this would also work with item.widget.setValue(10)

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.

Eh, the widget should inherently function as a text setter, so I'm fine with leaving it as w.setValue = w.setText.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Feel free to correct things in #1919 as you see fit, again, sorry for the trouble!

Our PR count is low enough we're getting more aggressive about small changes in lots of the library... this is going to keep happening until we effectively zero out the PR queue :( again, sorry for the headache!

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.

3 participants