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

Language server fixes #1

Merged
merged 4 commits into from
Jan 30, 2020
Merged

Language server fixes #1

merged 4 commits into from
Jan 30, 2020

Conversation

jages
Copy link
Contributor

@jages jages commented Jan 27, 2020

Description

This merges various changes from @NLueg (originially merged into openvalidation/openvalidation:develop) into openvalidation/openvalidation-rest, from before the migration to it's own repository.

There are siginificant changes to pom.xml, that require various changes.

Due to the changes made to the underlying API this requires openvalidation PR#57 to complete.

@jages
Copy link
Contributor Author

jages commented Jan 27, 2020

@thecodemonkey try merging this into master branch, results from maven look good.

NLueg
NLueg previously approved these changes Jan 28, 2020
Copy link

@NLueg NLueg left a comment

Choose a reason for hiding this comment

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

Looks good to me. I actually don't looked at every file but the implementation works perfectly with the monaco implementation.

@jages
Copy link
Contributor Author

jages commented Jan 29, 2020

@NLueg why is the port 31057 hard coded into application.properties? this is also not reflected in the docker file (resulting in no service from docker). please review.

@jages jages requested a review from NLueg January 29, 2020 14:44
@jages jages added the question Further information is requested label Jan 29, 2020
@NLueg
Copy link

NLueg commented Jan 30, 2020

@jages Well, that's a good point ... I hardcoded the port into application.properties because I needed to use a port that is surely not used on the system. This is required for example for the VSCode-Extension, because the backend runs in a separate process and I don't wanted to block a port like 8080.

But that's a good point with docker. What would you say would be a good solution for this problem? Of course, we just could add the port to docker but maybe you have a better idea for this.

@NLueg NLueg dismissed their stale review January 30, 2020 05:34

we need to fix the docker problem first

@jages
Copy link
Contributor Author

jages commented Jan 30, 2020

@jages Well, that's a good point ... I hardcoded the port into application.properties because I needed to use a port that is surely not used on the system. This is required for example for the VSCode-Extension, because the backend runs in a separate process and I don't wanted to block a port like 8080.

But that's a good point with docker. What would you say would be a good solution for this problem? Of course, we just could add the port to docker but maybe you have a better idea for this.

all settings in application.properties can also be passed as command line arguments.
i will either change the setting in the docker file, or just remove the port from the settings file and adjust the README accordningly. I will request a re-review once i have implemented these changes.

@jages
Copy link
Contributor Author

jages commented Jan 30, 2020

@NLueg please review the dependency of the specific port in the vs-code and npm package. I currently see no reason to specify the port in the config, as it can just be started in the CLI (especially in the npm package, this could be arranged to start the port with the listening port of the consumer)

@NLueg
Copy link

NLueg commented Jan 30, 2020

@jages I will fix the problem in both packages. But this doesn't block this PR, because the JAR is currently inserted manually in both packages. In my opinion, we can merge this.

@jages
Copy link
Contributor Author

jages commented Jan 30, 2020

This addresses #2 in a small capacity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants