Skip to content

Remove STRTransform main#2466

Merged
j9ac9k merged 4 commits intopyqtgraph:masterfrom
Jaime02:remove-main
Oct 29, 2022
Merged

Remove STRTransform main#2466
j9ac9k merged 4 commits intopyqtgraph:masterfrom
Jaime02:remove-main

Conversation

@Jaime02
Copy link
Copy Markdown
Contributor

@Jaime02 Jaime02 commented Oct 5, 2022

Why this file has a main function? It is not meant to be run independently, right? Shouldn't it be removed?

Why this file has a main function? It is not meant to be run independently, right? Shouldn't it be removed?
@Jaime02 Jaime02 closed this Oct 5, 2022
@Jaime02 Jaime02 deleted the remove-main branch October 5, 2022 19:24
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 5, 2022

Looks like the __main__ code there was added 12 years ago, we certainly don't use it, so I'm good with removing it.

That said, read-the-docs CI is failing on build and looks like it was complaining about a circular import; so I would suggest unwinding some of your changes.

EDIT: I see you closed the PR now; going to leave the comment just in case. Thanks for the submission!

@Jaime02
Copy link
Copy Markdown
Contributor Author

Jaime02 commented Oct 5, 2022

@j9ac9k oh so it is completely pointless? I have seen the test failing due to the imports, but I thought that the main function could have something to do with that 🤓 Then shall I remove it and try to fix it?

@Jaime02 Jaime02 restored the remove-main branch October 10, 2022 08:31
These files have main entry points that are pointless, therefore they should be removed
@Jaime02 Jaime02 reopened this Oct 10, 2022
Comment on lines 178 to +184
#def __div__(self, t):
#"""A / B == B^-1 * A"""
#dt = t.inverted()[0] * self
#return SRTTransform(dt)
#return SRTTransform.SRTTransform(dt)

#def __mul__(self, t):
#return SRTTransform(QtGui.QTransform.__mul__(self, t))
#return SRTTransform.SRTTransform(QtGui.QTransform.__mul__(self, t))

Check notice

Code scanning / CodeQL

Commented-out code

This comment appears to contain commented-out code.
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 10, 2022

Looking at the diff more closely, you have indeed introduced a circular import. SRTTransform.py does imports SRTTransform3d, and SRTTransform3d now imports SRTTransform.py, which causes a circular import.

@Jaime02
Copy link
Copy Markdown
Contributor Author

Jaime02 commented Oct 10, 2022

Looking at the diff more closely, you have indeed introduced a circular import. SRTTransform.py does imports SRTTransform3d, and SRTTransform3d now imports SRTTransform.py, which causes a circular import.

Yeah but I fixed that in the lastest patch. I used regular import x; x.y instead of from x import y @j9ac9k

@Jaime02
Copy link
Copy Markdown
Contributor Author

Jaime02 commented Oct 24, 2022

Hey @j9ac9k can you check this again please?

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 29, 2022

I wouldn't say I like importing just the module instead of the class itself, I suppose I prefer to import the class at the bottom of the file, which feels like a rubbish way to go about handling the circular import. Both these classes should likely reside in the same module, but that's well outside the scope of this PR.

Thanks for the PR @Jaime02 Merging.

@j9ac9k j9ac9k merged commit 2ed2551 into pyqtgraph:master Oct 29, 2022
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.

2 participants