-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
#1708 - Replace original implementation and remove copyright info from license file #6195
Conversation
…fo from license file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
kogito-apps GHA test failure can be ignored as it's being worked on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @porcelli . Always good to see we are able to simplify the codebase we maintain.
I have some comments and questions before merge.
@@ -234,17 +234,6 @@ for drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/Ja | |||
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF | |||
THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||
|
|||
------------------------------------------------------------------------------------ | |||
for drools-decisiontables/src/main/java/org/drools/decisiontable/parser/csv/CsvLineParser.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, update also https://github.com/apache/incubator-kie-drools/blob/main/.rat-excludes, we need to remove the entry CsvLineParsrer
from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
if (line.isEmpty()) { | ||
fields.add(""); | ||
return fields; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In past, we used often Collections.singletoneList
, what is your opinion about:
if (line.isEmpty()) {
return Collections.singletonList("");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I'll adjust
public CsvLineParser(char delimiter) { | ||
this.delimiter = delimiter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep this constructor? I was not able to find its usage, to me it seems we use only the default one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I wasn't completely sure and that's why cheated and sent this way so to have the CI tell me that if in the codebase we had this used somehow :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updates @porcelli
…fo from license file (apache#6195) * apache#1708 - Replace original implementation and remove copyright info from license file * incorporating PR review feedback
Closes: apache/incubator-kie-issues#1708