[276] Refactor API #277
[276] Refactor API #277
Conversation
to provide a stable API with v2.0.0
ghentschke
force-pushed
the
refactor-API
branch
from
afd0b0f to
f4a548d
Compare
There was a problem hiding this comment.
Approve - but there seems to be compilation failures that need resolving first.
I will rely on @ruspl-afed to do a review too as he normally has deeper looks at specifics of the APIs that I do.
| Export-Package: org.eclipse.cdt.lsp, | ||
| org.eclipse.cdt.lsp.config;x-friends:="org.eclipse.cdt.lsp.clangd", |
There was a problem hiding this comment.
Should this have internal in the name if it is only intended to be used by friends? Or do you have 3 levels:
- Public API: no internal in names and no x-friends/x-internal
- Internal API (between bundles): no internal in names, and x-friends/x-internal
- complete non-API: not included in Export-Package
There was a problem hiding this comment.
Public API: no internal in names and no x-friends/x-internal
Internal API (between bundles): no internal in names, and x-friends/x-internal
complete non-API: not included in Export-Package
yes, that's what I had in mind
There was a problem hiding this comment.
internal segment in names comes from this [relatively unclear] convention
ghentschke
force-pushed
the
refactor-API
branch
from
3e3ab9a to
2f7e5ab
Compare
|
I fixed the unit tests. Please review @ruspl-afed |
| Export-Package: org.eclipse.cdt.lsp, | ||
| org.eclipse.cdt.lsp.config;x-friends:="org.eclipse.cdt.lsp.clangd", |
There was a problem hiding this comment.
internal segment in names comes from this [relatively unclear] convention
There was a problem hiding this comment.
Questionable type even for component-internal API. It exposes a lot of needless UI API. Let's focus on what is really needed from it
There was a problem hiding this comment.
It exposes a lot of needless UI API
Yes ,we can do that in the next API refactor iteration
| @@ -9,7 +9,7 @@ | |||
| * Contributors: | |||
| * See git history | |||
| *******************************************************************************/ | |||
| package org.eclipse.cdt.lsp; | |||
| package org.eclipse.cdt.lsp.util; | |||
There was a problem hiding this comment.
Traditional, but weak name for package and type, I'm sure we can have something better here.
There was a problem hiding this comment.
Also, next API refactor iteration
| @@ -11,14 +11,12 @@ | |||
| * Alexander Fedorov (ArSysOp) - rework to OSGi component | |||
| *******************************************************************************/ | |||
|
|
|||
| package org.eclipse.cdt.lsp.internal; | |||
There was a problem hiding this comment.
component implementation should never be an API, even if we are talking about internal "cdt-lsp" scope
|
@ghentschke , what is your plan regarding this one? My suggestion is to merge it "as is" and then have another iteration of API refinement. |
I am currently working on another issue but I wanted to catch up some of your comments which can be fixed fast. I think I can do that today. |
|
I would at least fix the build error in the test |
|
@ruspl-afed or @jonahgraham Do have na idea why the master cannot be build? It claims that the maven version must be 3.9.0. But the maven version has already been updated here: |
|
This is controlled by https://github.com/eclipse-cdt/cdt-lsp/blob/master/Jenkinsfile and not the GHA file. Unfortunately, I have no idea how your docker image is defined to be able to help you more. In not that custom environments smth like https://github.com/eclipse-platform/eclipse.platform.ui/blob/04dd9a1bf255b0af610082a38d79ccd4fa861084/Jenkinsfile#L11 is enough. |
|
Probably changing the mvn executable used to the one that cdt uses https://github.com/eclipse-cdt/cdt/blob/95fe4d87010b1446741bedd8818bea3252ebc8be/Jenkinsfile#L40C23-L40C61 should do the trick. |
|
Sorry - @ghentschke I forgot that I had updated only main CDT Jenkinsfile and not the cdt-lsp one. PR incoming. |
|
@akurtakov Thank you for the hints! |
to provide a stable API with v2.0.0