Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add appModule for Spring Tool Suite 4 #10339

Merged
merged 5 commits into from Dec 3, 2019

Conversation

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Oct 6, 2019

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

This comment has been minimized.

Copy link

AppVeyorBot commented Oct 6, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit 1566466dff

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Oct 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Oct 6, 2019

@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

This comment has been minimized.

Copy link

alexandretoco commented Oct 6, 2019

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Oct 6, 2019

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

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Oct 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Oct 7, 2019

@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

source/appModules/springtoolsuite4.py Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Oct 8, 2019

@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 Oct 10, 2019
@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Oct 26, 2019

@leonardder You have pending review here.

source/appModules/springtoolsuite4.py Outdated Show resolved Hide resolved
@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Oct 31, 2019

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

@lukaszgo1

This comment has been minimized.

Copy link
Contributor Author

lukaszgo1 commented Nov 30, 2019

@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.

Copy link
Contributor

feerrenrut left a comment

Thanks @lukaszgo1

@feerrenrut feerrenrut merged commit 42ea5c5 into nvaccess:master Dec 3, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@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 lukaszgo1:I10001 branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.