GeezerVison

@wumpz: Please check out the great work on PR #1254. It adds a new token QUICK and which of course should not become a Reserved Keyword.

Of course the author @OlivierCavadenti has no chance to know, that he would need to add QUICK to the productions RelObjectNamexxxx() in order to enable this token as Object Identifier. (And how should he, I figured that out only 6 month after my first contribution.)

This PR would solve this challenge reliably, preventing new tokens from breaking valid statements and ease the entry for new contributors.

The naming of this RelObjectName ... methods is sure an issue that should be adressed in futher improvements. So sorry for that: its grown ... ;)

I am not a fan of injecting those keywords, since not every keyword could be introduced this way and needs other parts of the grammar to be modified. However, building this keyword testing using JUnit 5 is cool.

The naming of this RelObjectName ... methods is sure an issue that should be adressed in futher improvements. So sorry for that: its grown ... ;)

You would do me a great favor by documenting the purpose of those 5 methods verbosely. I tried my best to figure it out by trial'n error but I am still not exactly sure why we ended up with 5 methods. (And no worries about grown code.)

I am not a fan of injecting those keywords, since not every keyword could be introduced this way and needs other parts of the grammar to be modified. However, building this keyword testing using JUnit 5 is cool.

Honestly, I am not a big fan of that PR either -- but after spending many days on that matter trying various things it was the best approach I found and in my opinion it is still much better than the current state:

1. at the moment, Keywords are just a mess and we get a lot of issues about keywords only.
   Are you able to document right now, what keywords are reserved and for what reason? I do not think so and the PR would solve this documentation problem.

2. today, even when you have an opinion on keywords, we have no parametrized tests.
   You kindly admit, that these tests are kind of cool and I appreciate your feedback. Those parametrized tests however depend on a well defined list of keywords, which the PR provides. (Although we can argue of course, how/where exactly such a list should be defined.)

not every keyword could be introduced this way and needs other parts of the grammar to be modified

3. While that is true for the PR, it is also true by now already. Also we could easily enforce a policy of "Keywords are defined by simple Tokens" (not allowing complex Tokens for Keywords).

The work flow is to: a) define the Tokens and Productions in the Grammar first and then b) document any new RESERVED Keywords only in the List (emphasis on RESERVED).

The PR has one big advantaged: By defining RESERVED keywords only, it will determine automatically any allowed Keywords and modify the Grammar semi-automatically.

Allow me to ask please:

1. what was your preferred way to manage and document RESERVED KEYWORDs

2. what was your preferred way to maintain any allowed Token/Keyword in RelObjectName()

3. what was your preferred way to test all the possible Keywords (automatically)

And can we have a Skype chat about this :-)

Since you only have a list of all whitelisted keywords in ParserKeywordsUtil are not detected. So every addition of tokens needs to be recognized by a developer.

No, it is exactly the opposite: Restricted Keywords are rather static and would not be touched normally. Developer adds to All Keywords only by adding tokens/productions to the Grammar. Then the Gradle task builds Whitelist Keywords automatically and confirms them by brute force testing.

Only when those tests fail, we would have to amend Restricted Keywords. This could even be automated, it is simple Java Code after all (or could be a Text file or whatever.)

Where I still fail to understand you plan is this point:

1. We agree to run the brute force tests in order to identify/confirm Restricted Keywords, do we?
2. But those tests will work only AFTER updating the Whitelisted Keyword in RelObjectNameWithoutValue() -- because the tests rely on the final grammar.

So how would you run your Tests without whitelisting in the grammar first?

So how would you run your Tests without whitelisting in the grammar first?

Simply this test will fail until you whitelist or restrict the new tokens. That's what forces developers to do this decision (for us). You are right, that the restricted list should change rarely. The test should then give an advice, how and were this change should be done.
I am a fan of brute force testing changes a developer (can) make. In a sense this is the same like enforcing a specific code style.

No, it is exactly the opposite: Restricted Keywords are rather static and would not be touched normally. Developer adds to All Keywords only by adding tokens/productions to the Grammar. Then the Gradle task builds Whitelist Keywords automatically and confirms them by brute force testing.

I was not aware of this automatic whitelist keyword building. So this is +1.

So are you going to resolve the conflicts? This improvement of parsing could be done afterwards.

@wumpz When could this pr be merged? The parse error is really annoying.

@wumpz When could this pr be merged? The parse error is really annoying.

I can supply you with a bleeding edge JAR file including those PRs, in case its urgent.

So after merging, I am not able to build anymore since ConditionalKeywordsTest fails. I merged another PR.

So how should I proceed if I am not willing to use gradle? Isn't that the same problem you had with my proposal? Maven is the main build pipeline.

Good Morning. Thank you for finally accepting the PR.

What caused the problem:

1. you have accepted another PR before (introducing the new Token LOCKED)
2. you have not added LOCKED to the white- or black-list

Solution:
Amend the white- or black-list and then run the buildKeywords Task.

This should have been done by the author of the LOCKED PR from now on any author will be forced to do so.

It is exactly as you have requested for in your explanations.

Although I will run through this procedure today and send a very small PR.

So how should I proceed if I am not willing to use gradle?

If you insist in not using gradle updateKeywords then you still can amend RelObjectNameWithoutValue manually and add the whitelisted new token -- same as it has always been done before.

I would also provide an equivalent Maven task soon, since we are moving forward now.

Workflow should be like that:

1) Amend Tokens in the Grammar
2) When keyword-tests fail, run gradle updateKeywords
3) Then build via mvn package or gradle build (it does not matter)
4) Push/Deploy

Using gradle updateKeywords task is optional, you can also add the new Tokens manually has it has always been done before.

After accepting the Keywords PR, any further PR will automatically fail acceptance tests, unless keywords are in order.

No developer that is using maven knows what to do here? So this workflow has somehow to be included in a developer help. At the moment this is only your tool, since nobody knows and not everybody uses gradle.

Since we are not willing to run the gradle task and your tests use ParserKeywordsUtils.getReservedKeywords how should updating the grammar be enough?

Additional since some JavaCC methods are in place now there is a direct dependency for JavaCC.

Additional since some JavaCC methods are in place now there is a direct dependency for JavaCC.

Yes, because you insisted to parse the Grammer using JavaCC instead of using Regular Expressions. I suggest to use Regular Expressions instead for that reason.

Since we are not willing to run the gradle task and your tests use ParserKeywordsUtils.getReservedKeywords how should updating the grammar be enough?

Because this is exactly what you would have done before the semi-automation: edit the Production RelObjectNameWithoutValue manually. You still could very much do that and we should keep that option at all cost.

That said, I have added a Maven Task for running the same updateKeywords task.
You can run it via:

mvn compile exec:java

No developer that is using maven knows what to do here? So this workflow has somehow to be included in a developer help. At the moment this is only your tool, since nobody knows and not everybody uses Gradle.

Yes, of course. It has been verbosely documented within the new website, which I have committed 1 month ago or so: https://manticore-projects.com/JSQLParser/contribution.html#manage-reserved-keywords

I have updated the Keywords2 PR #1653 accordingly with your requested Maven task.

The Maven task could be implemented as a mojo (maven-plugin) I think, are you interested in that? Then you can run this plugin from the pom.xml during, for example, the generate-sources phase.

I have some experience writing these maven plugins, so if I infd some time this week, maybe I could draft up some integration so it 'just works' when you run for example mvn compile

But probably this will work as well:

<plugin>
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>exec-maven-plugin</artifactId>
        <executions>
            <execution>
                <id>codegeneration</id>
                <phase>generate-resources</phase>
                <goals><goal>java</goal></goals>
                <configuration>
                   <mainClass>com.codegenerator.CodeGeneratorApplication</mainClass>
                </configuration>
            </execution>
        </executions>

</plugin>

Thanks! I have add submitted another PR like that on Saturday already.

Ah excellent!
I'll have to catch up I see :)

I can't find that PR? I think there might also be a chicken-and-egg problem, as the java code that does the work, also needs to be compiled first? (you can solve that using multi-module maven build, but that requires a bit more refactoring)

this one: #1653

Its pretty straight forward actually since we can execute Java Code directly without compiling the whole package. Also, this step is executed on demand: only when new tokens introduced and/or the Keyword Tests fail.

Yes, because you insisted to parse the Grammer using JavaCC instead of using Regular Expressions. I suggest to use Regular Expressions instead for that reason.

That's why e.g. my proposal is only running in scope test. Then this dependency is not needed. I was talking about the IMHO right way to parse the grammar and not the context in which this parsing should happen. I thought this should be obvious. For the same reason, the JSqlParser jar should not include this generation class. It is part of the build process (I know you do not accept that, but I think it's evident.) So a quick solution would be to put all of it in the test folder and make this JavaCC dependency of scope test.

However, to get a clean build again use for now Regular Expressions.

Saying that, since this is a build step or could be one, it should be separated from JSqlParser source code and put in some kind of build helper module and then could be definitely used if JSqlparser is built. Now If you clone this project you will fail, since the generator class is not yet built.

I have updated the Keywords2 PR #1653 accordingly with your requested Maven task.

I have reviewed it already and found in this issue the needed information on how to run it, so I will merge it.