jakartaee / expression-language

Jakarta Expression Language
https://eclipse.org/ee4j/el
Other
67 stars 49 forks source link

[issue-78] EL expressions that contain unnecessary parentheses fail #79

Closed tmiyargi closed 4 years ago

tmiyargi commented 5 years ago

Added change from https://svn.apache.org/viewvc?view=revision&revision=1571242 Regenerated the parser and added a test.

tmiyargi commented 5 years ago

All changes are now in place, headers included and I have signed the Contributor Agreement.

markt-asf commented 5 years ago

I did obtain clarification that, for auto-generated files, the license of the output should be the same as the license for the input.

The additional licensing changes don't look right to me but I am not sufficiently well-versed in Eclipse procedures for pulling in ALv2 licensed content to determine if the proposed PR has done this correctly.

bshannon commented 5 years ago

@markt-asf It looks like you were the author of the original fix to the Apache code. Why don't you just apply that fix, which you still own, to the EE4J code, under the Eclipse license?

tmiyargi commented 5 years ago

@markt-asf @bshannon should I close this PR then so you can add the changes yourselves?

bshannon commented 5 years ago

@markt-asf, it's up to you. Can you just apply your fix, under the Eclipse license?

DennisBayer commented 5 years ago

Hi there,

any updates on this issue @markt-asf ? Additionally, is there any timeline for a release which would contain this fix?

Best regards Dennis

tmiyargi commented 5 years ago

I've reworked the fix so it is no longer a copy. I hope you can merge it now.

bshannon commented 5 years ago

It looks like @markt-asf has checked out of this project since he's not responding. I've dismissed his review. We should consider removing him as a Committer.

The latest changes contain a lot of gratuitous formatting changes, is that because these files were regenerated? Unfortunately, that makes it harder to review the changes.

In any event, this needs review by someone more familiar with this code. I've added some other reviewers who might be able to help.

tmiyargi commented 5 years ago

@bshannon Two files have been changed the others are regenerated using JavaCC. Thank you for looking into this.

markt-asf commented 5 years ago

My interest in EL at Eclipse is limited to the API. Contributing to the implementation is not an itch I have any motivation to scratch. The sole reason for my commenting on this PR was to point out that the original PR did not meet the requirements of the open source license of the project from which it was copied.

bshannon commented 5 years ago

The sole reason for my commenting on this PR was to point out that the original PR did not meet the requirements of the open source license of the project from which it was copied.

Which you could've solved by contributing your fix to this project as well. That would've been simple for you to do. It's disappointing that you're not willing to do so.

tmiyargi commented 4 years ago

Done rebase and solved conflicts, @arjantijms, @ruolli could you have a look?

bmaxwell commented 4 years ago

@arjantijms / @ruolli any updates on if this change is acceptable?

rmartinc commented 4 years ago

Hi,

The only real changes in this PR are in the following files:

All the other changes are result of executing the ant with the file impl/build.xml, which in turns executes JavaCC and regenerates the Java files for the EL parser. Maybe you feel more comfortable if only the modified files are included in the PR, the review will be easier. Later you can re-execute the ant and re-generate the auto-generated files by your own. WDYT?

rmartinc commented 4 years ago

Hi,

Sorry again for asking about this, but can we do something to move the PR forward? I proposed to send just one commit but maybe it's better if two commits are sent in the PR (the real modifications and then another one with the automatic ones). @tmiyargi cannot work more on the issue, so we'll need to send another PR for this.

Thanks!

rmartinc commented 4 years ago

Waiting for 3.0.4 relase then. Thanks a lot @bshannon !!!