search

CARiSMA-Tool / carisma-tool

Implementation of the CARiSMA Tool
https://github.com/CARiSMA-Tool/carisma-tool
Eclipse Public License 1.0
3 stars 5 forks source link

Replaced base64 check with use of Java standard lib #44

Closed SvenPeldszus closed 6 months ago

SvenPeldszus commented 6 months ago

Due to the updatesite not being available, I tried to build carisma on my own and had issues with an apache codecs library not available in Eclipse 2024-03. This lib was only used for checking whether a String is BASE64 encoded. I replaced this with an implementation only using the Java standard library.

SvenPeldszus commented 6 months ago

Probably the code could also be removed without replacement.

nuthub commented 6 months ago

Hi, did you run ./mvnw clean install (or ./mvnw clean verify) before trying to build CARiSMA? (See also https://github.com/CARiSMA-Tool/carisma-tool/blob/master/documentation/development.md). It pulls external dependencies that are defined in pom.xmls

However, I think it's generally a good idea to remove external dependencies, if possible.

SvenPeldszus commented 6 months ago

It was not one of the dependencies from maven central, which would have been downloaded in both variants anyways, but some apache library provided as Eclipse plugin.

nuthub commented 6 months ago

ah, I see. I thought you were referring to org.apache.commons.lang3 (and not org.apache.commons.codec).

nuthub commented 6 months ago

I have no issues to build CARiSMA from current master and GitHub Actions is also able to build the project. I am talking about building via Maven, which still involves Eclipse's 2023-06 Update Site. I assume you fail to build CARiSMA from within Eclipse?

I'd like to make GitHub notify us about that issue, but it seems that Tycho finds the bundle in the 2023-06 repository, while Eclipse itself doesn't (even after adding 2023-06 Update Site), if the bundle/plugin is not installed via some feature.

Probably the code could also be removed without replacement.

I also see no reason to keep the ContentFactory. Do you have any objections to just removing it?

sonarcloud[bot] commented 6 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

SvenPeldszus commented 6 months ago

I'd like to make GitHub notify us about that issue, but it seems that Tycho finds the bundle in the 2023-06 repository, while Eclipse itself doesn't (even after adding 2023-06 Update Site), if the bundle/plugin is not installed via some feature.

This was the same for me

I also see no reason to keep the ContentFactory. Do you have any objections to just removing it?

I think it stems from the VisiOn project and should not be used anywhere besides that. I am not sure if this part of CARISMA is still in use. If it is on a branch, one could migrate the fixed version there to make it later easier to reactivate this part if needed. Thereby, maybe explicitly moving it into one of the VisiOn-related plugins.

nuthub commented 6 months ago

I think it stems from the VisiOn project and should not be used anywhere besides that. I am not sure if this part of CARISMA is still in use. If it is on a branch, one could migrate the fixed version there to make it later easier to reactivate this part if needed. Thereby, maybe explicitly moving it into one of the VisiOn-related plugins.

I removed the ContentFactory from master. Would you mind to create a separate PR for your patch against the vision branch (or directly merge your patch into it)?