Skip to content

Add appModule for Spring Tool Suite 4#10339

Merged
feerrenrut merged 5 commits into
nvaccess:masterfrom
lukaszgo1:I10001
Dec 3, 2019
Merged

Add appModule for Spring Tool Suite 4#10339
feerrenrut merged 5 commits into
nvaccess:masterfrom
lukaszgo1:I10001

Conversation

@lukaszgo1

@lukaszgo1 lukaszgo1 commented Oct 6, 2019

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #10001

Summary of the issue:

Spring Tool Suite 4 changed name of its main executable. Because of this proper appModule wasn't loaded.

Description of how this pull request fixes the issue:

Add app Module for a proper executable. This simply imports module for Eclipse.

Testing performed:

Try build tested by @alexandretoco. He confirms that proper module is loaded. I cannot test myself because Spring Tool suite fails to start on my machine for some reason.

Known issues with pull request:

None known

Change log entry:

Section: Bug fixes

Spring Tool Suite Version 4 is now supported

@AppVeyorBot

Copy link
Copy Markdown

PR introduces Flake8 errors 😲

See test results for Failed build of commit 1566466dff

@lukaszgo1

Copy link
Copy Markdown
Contributor Author

This PR causes two Flake8 errors: F403 and F401. Can I simply ignore these with an appropriate noqa? All app modules importing module for different program are doing exactly the same. cc @feerrenrut

@lukaszgo1

Copy link
Copy Markdown
Contributor Author

@alexandretoco Could you please download this build, temporarily remove module that I've send you privately from the scratchpad directory and confirt that the issue is fixed?

@alexandretoco

alexandretoco commented Oct 6, 2019 via email

Copy link
Copy Markdown

@lukaszgo1

Copy link
Copy Markdown
Contributor Author

The add-on that you ate using is not compatible with Python 3, so this isn't caused by this PR.

@feerrenrut

Copy link
Copy Markdown
Contributor

Can I simply ignore these with an appropriate noqa?

In most cases, despite it breaking consistency, I would prefer just what is used be imported explicitly. In this case, because the goal is to map a new executable name to the same app module without having to maintain another file then I agree import * is the least brittle way. However please make sure you add a comment to explain why this #noqa is an exception.

I would really like app modules to be mapped via a more flexible mechanism than by matching the python file name with the executable.

@lukaszgo1

Copy link
Copy Markdown
Contributor Author

@feerrenrut Done, I hope my commend is clear enough. I'd be nice to have this merged in time for 2019.3 because without this fix latest version of Spring Tool isn't really usable according to @alexandretoco

Comment thread source/appModules/springtoolsuite4.py Outdated
@LeonarddeR

Copy link
Copy Markdown
Collaborator

@feerrenrut wrote:

I would really like app modules to be mapped via a more flexible mechanism than by matching the python file name with the executable.

see #10248, which is related to that desire.

@lukaszgo1 lukaszgo1 requested a review from LeonarddeR October 10, 2019 07:36
@lukaszgo1

Copy link
Copy Markdown
Contributor Author

@LeonarddeR You have pending review here.

Comment thread source/appModules/springtoolsuite4.py Outdated
@lukaszgo1

Copy link
Copy Markdown
Contributor Author

@feerrenrut This is ready for a review if you have time.

@lukaszgo1

Copy link
Copy Markdown
Contributor Author

@feerrenrut Would you be able to review this, and if it looks ok get it into 2019.3? I understand that we are quite close to the release, however the change itself is very small and without it someone depending on latest version of Spring Tool has to wait a few more months.

@feerrenrut feerrenrut left a comment

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.

Thanks @lukaszgo1

@feerrenrut feerrenrut merged commit 42ea5c5 into nvaccess:master Dec 3, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Dec 3, 2019
feerrenrut added a commit that referenced this pull request Dec 3, 2019
@lukaszgo1 lukaszgo1 deleted the I10001 branch December 3, 2019 14:53
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.

redundant speech when navigating trees in eclipse

6 participants