Skip to content
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

cEP-0018: Integration of ANTLR in coala #124

Merged
merged 1 commit into from Jun 6, 2018

Conversation

@virresh
Copy link
Member

commented May 6, 2018

Propose integration of ANTLR in coala

Closes #118

cEP-0018.md Outdated
flexible visitor based method of AST traversal (as opposed to listener based
mechanisms).

The proposal is to introduce a paralled concept to the coala-bears library,

This comment has been minimized.

Copy link
@palash25

palash25 May 7, 2018

Member

paralled to paralleled

cEP-0018.md Outdated
Here is the detailed implementation stepwise:

1. There will be a separate repository named as coala-ast which will be
installable via ```pip install coala-ast```.

This comment has been minimized.

Copy link
@palash25

palash25 May 8, 2018

Member

indent all the points 👍

@virresh virresh force-pushed the virresh:virresh/cEP branch from e2be60c to 9561ece May 8, 2018

cEP-0018.md Outdated

| Metadata | |
| ------------ |-----------------------------------------|
| cEP | 0018 |

This comment has been minimized.

Copy link
@palash25

palash25 May 8, 2018

Member

change that to 18 no need for the two 0's

This comment has been minimized.

Copy link
@Udayan12167

Udayan12167 May 8, 2018

Contributor

+1

This comment has been minimized.

Copy link
@jayvdb
cEP-0018.md Outdated
| Status | Proposed |
| Type | Feature |

# Abstract

This comment has been minimized.

Copy link
@palash25

palash25 May 8, 2018

Member

# should just be for the title and ## for the different sections imo

cEP-0018.md Outdated
| Metadata | |
| ------------ |-----------------------------------------|
| cEP | 0018 |
| Version | 0.1.0 |

This comment has been minimized.

Copy link
@Udayan12167

Udayan12167 May 8, 2018

Contributor

Version should be 1.0

cEP-0018.md Outdated

1. There will be a separate repository named as coala-ast which will be
installable via ```pip install coala-ast```.
2. A new interface would be introduced in coala-core (```coala/coalib```),

This comment has been minimized.

Copy link
@Udayan12167

Udayan12167 May 8, 2018

Contributor

I don't understand what you mean by a new interface.

This comment has been minimized.

Copy link
@virresh

virresh May 9, 2018

Author Member

By a new interface, I meant a new package that could be imported in order to use the ASTWalker and ASTLoader classes
Will change interface -> package to avoid confusion 😅

cEP-0018.md Outdated
installable via ```pip install coala-ast```.
2. A new interface would be introduced in coala-core (```coala/coalib```),
which would be the new ast package. This would be the ```coalib.ast``` package
3. The ```coalib.ast``` package will provide endpoints relevant to the visitor

This comment has been minimized.

Copy link
@Udayan12167

Udayan12167 May 8, 2018

Contributor

Okay so as discussed we don't want any ANTLR dependencies in core coala. Thus coalib might not be the right place for this.

The whole point of creating a new repository is that you can have a folder in that repo called astlib which could then be used to write ast bears in that repository.

This comment has been minimized.

Copy link
@virresh

virresh May 9, 2018

Author Member

Ah
I misunderstood
I thought we wanted to maintain a separate library for parsers, (similart to the antlr4-grammars repository) and have the walker and visitor inside of coala-core

Will update 😅

cEP-0018.md Outdated
which would be the new ast package. This would be the ```coalib.ast``` package
3. The ```coalib.ast``` package will provide endpoints relevant to the visitor
model of traversing the ast generated via ANTLR.
4. The coala-ast repository will hold parser generated (with python targe)

This comment has been minimized.

Copy link
@Udayan12167

Udayan12167 May 8, 2018

Contributor

Can you do some research on how much time does it take to generate a parser. We don't want code bloat if the time is short to generate one. We can just generate on the fly is CI and on the user machine.

This comment has been minimized.

Copy link
@virresh

virresh May 9, 2018

Author Member

The time required to generate a parser on my machine for one single language for a python target isn't much....

$ time antlr4 -Dlanguage=Python3 Python3.g4 

real	0m1.184s
user	0m3.328s
sys	0m0.112s

But the bigger drawback for making a parser on the fly is we would be mandating the user machine to have a compatible JAVA version installed and working so that the antlr's jar file can be called and executed (though on the CI it won't be a problem I believe)
I couldn't come up with an elegant way to go about this...

@ksdme
Copy link
Member

left a comment

The commit message is missing a full stop.

cEP-0018.md Outdated
1. There will be a separate repository named as coala-ast which will be
installable via ```pip install coala-ast```.
2. A new interface would be introduced in coala-core (```coala/coalib```),
which would be the new ast package. This would be the ```coalib.ast``` package

This comment has been minimized.

Copy link
@jayvdb

jayvdb May 9, 2018

Member

This would be the coalib.ast package

How is coala-ast going to load code into coalib.ast which is owned by the coala package?

I believe it cant use https://www.python.org/dev/peps/pep-0420/ as coalib has stuff in the top level package module.

This comment has been minimized.

Copy link
@virresh

virresh May 9, 2018

Author Member

😅
by coala/coalib I meant the location in the repository and coalib.ast was the way the library would be imported
will make it clearer in the cEP : )

cEP-0018.md Outdated
Here is the detailed implementation stepwise:

1. There will be a separate repository named as coala-ast which will be
installable via ```pip install coala-ast```.

This comment has been minimized.

Copy link
@jayvdb

jayvdb May 9, 2018

Member

maybe put 'antlr' in the name of the repo and package?

we are also doing a pyflakes ast project this year, and there are clang ast bears.

And the future may bring other ast which are incompatible with the antlr ast.

This comment has been minimized.

Copy link
@virresh

virresh May 9, 2018

Author Member

Oh
Sure : )

@jayvdb

This comment has been minimized.

Copy link
Member

commented May 9, 2018

Rebase please, and ensure it passes the new rules otherwise your PR will be full of errors.

@virresh virresh force-pushed the virresh:virresh/cEP branch from 9670acd to 7d213b4 May 9, 2018

cEP-0018.md Outdated
Here is the detailed implementation stepwise:

````
1. There will be a separate repository named as coala-antlr-ast which will

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot May 9, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpsh72ob5m/cEP-0018.md
+++ b/tmp/tmpsh72ob5m/cEP-0018.md
@@ -30,24 +30,24 @@
 Here is the detailed implementation stepwise:
 

-1. There will be a separate repository named as coala-antlr-ast which will

  • be installable via pip install coala-antlr-ast.
    -2. A new package would be introduced in the coala-antlr-ast repository,
  • which would be the maintain all the dependencies for antlr-ast based
  • bears. This would be the antlrastlib package
    -3. The antlrastlib package will provide endpoints relevant to the
  • visitor model of traversing the ast generated via ANTLR.
    -4. Another package named antlrparsers will be created inside
  • coala-antlr-ast repository. This package will hold parsers generated
  • for various languages generated (with python target) beforehand for the
  • supported set of grammars.
    -5. antlrastlib will have an AST loader class that will be responsible
  • for loading the AST of a given file depending on the extension.
  • For e.g, a .c file will be loaded using the parser from
  • antlrastlib for c language.
    -6. Another AST walker class will be responsible for providing endpoints for
  • traversing the AST using methods such as get_next_node and
  • get_previous_node.
    1. There will be a separate repository named as coala-antlr-ast which will
  •   be installable via ```pip install coala-antlr-ast```.
    
    1. A new package would be introduced in the coala-antlr-ast repository,
  •   which would be the maintain all the dependencies for antlr-ast based
    
  •   bears. This would be the ```antlrastlib``` package
    
    1. The antlrastlib package will provide endpoints relevant to the
  •   visitor model of traversing the ast generated via ANTLR.
    
    1. Another package named antlrparsers will be created inside
  •   coala-antlr-ast repository. This package will hold parsers generated
    
  •   for various languages generated (with python target) beforehand for the
    
  •   supported set of grammars.
    
    1. antlrastlib will have an AST loader class that will be responsible
  •   for loading the AST of a given file depending on the extension.
    
  •   For e.g, a .c file will be loaded using the parser from
    
  •   ```antlrastlib``` for c language.
    
    1. Another AST walker class will be responsible for providing endpoints for
  •   traversing the AST using methods such as ```get_next_node``` and
    
  •   ```get_previous_node```.
    

## Management of the new `coala-ast` repository
```

This comment has been minimized.

Copy link
@virresh

virresh May 9, 2018

Author Member

I swear I did ran coala locally before pushing 😞

(coala) viresh@viresh-PC:/mnt/Common/Earlier/GIT/coala/cEPs$ git log -1
commit 7d213b4d31f685726bcdc5c8431348cf83eeebbe
Author: Viresh Gupta <viresh16118@iiitd.ac.in>
Date:   Sun May 6 14:31:17 2018 +0530

    cEP-0018: Integration of ANTLR in coala
    
    Propose integration of ANTLR in coala.
    
    Closes https://github.com/coala/cEPs/issues/118
(coala) viresh@viresh-PC:/mnt/Common/Earlier/GIT/coala/cEPs$ coala
Executing section spacing...
Executing section markdown...
Executing section commits...
Executing section cli...
(coala) viresh@viresh-PC:/mnt/Common/Earlier/GIT/coala/cEPs$ 

This comment has been minimized.

Copy link
@jayvdb

jayvdb May 9, 2018

Member

The fact you only have one gitmate error is evidence that you tried ;-)

Note gitmate's error message above is very strangely formatted for a 'patch'. A bug to raise in gitmate? :P

cEP-0018.md Outdated
1. There will be a separate repository named as `coala-antlr` which will
be installable via `pip install coala-antlr`.
2. A new package would be introduced in the coala-antlr repository,
which would be the maintain all the dependencies for antlr-ast based

This comment has been minimized.

Copy link
@saksham189

saksham189 May 31, 2018

Contributor

which would maintain

This comment has been minimized.

Copy link
@saksham189

saksham189 May 31, 2018

Contributor

antrl-ast in backticks

@virresh virresh force-pushed the virresh:virresh/cEP branch from 7f00609 to fd2c467 May 31, 2018

@Udayan12167

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

ack fd2c467

cEP-0018.md Outdated
2. The cib tool can be enhanced to deal with the installation of bears that
require only some specified parsers (for e.g a `PyUnusedVarBear` would
only require parser for python).
3. The cib tool can also trigger specialised builds and download the newly

This comment has been minimized.

Copy link
@jayvdb

jayvdb Jun 6, 2018

Member

this seems to be a lot of unnecessary effort, and means users will be trying to do stuff which the probably dont have permission to do, and we dont want them do, otherwise we have to answer why it doesnt work.

We can have nightly cron jobs on the repo, there by regularly updating the parsers.

cEP-0018.md Outdated
# classes that inherit ASTWalker within this module
# and return that class whose supported language matches
class ASTLoader():

This comment has been minimized.

Copy link
@jayvdb

jayvdb Jun 6, 2018

Member

() -> (object)

cEP-0018.md Outdated
}
@staticmethod
def loadFile(file,filename,lang = 'auto'):

This comment has been minimized.

Copy link
@jayvdb

jayvdb Jun 6, 2018

Member

pep8 naming. also run this code through pycodestyle and pydocstyle. It is a mess.
(... is valid syntax, but they need to be put in the appropriate indent levels.)

@jayvdb

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

unack fd2c467

@virresh virresh force-pushed the virresh:virresh/cEP branch from fd2c467 to 10c6b1b Jun 6, 2018

cEP-0018.md Outdated
self.tree = ASTLoader.load_file(file, filename)
self.treeRoot = self.tree
...

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -153,7 +153,6 @@
         self.tree = ASTLoader.load_file(file, filename)
         self.treeRoot = self.tree
     ...
-

Prototype of Py3ASTWalker

cEP-0018.md Outdated
since / and // are different in py3, this is py3 specific
"""
...

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -199,7 +199,6 @@
         since / and // are different in py3, this is py3 specific
         """
     ...
-

Prototype of ASTBear class implementation

cEP-0018.md Outdated
):
"""Actual run method - to be implemented by bear."""
raise NotImplementedError('Needs to be done by bear')

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -240,7 +240,6 @@
             ):
         """Actual run method - to be implemented by bear."""
         raise NotImplementedError('Needs to be done by bear')
-

A test bear

cEP-0018.md Outdated
wherever required
"""
...

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -274,5 +274,4 @@
             wherever required
             """
         ...
-
-```
+```
@virresh

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2018

The newlines at the end of file required by PyCodeStyleBear cause the markdown bear to complain
So removing those newlines

@virresh virresh force-pushed the virresh:virresh/cEP branch from 10c6b1b to 861c8eb Jun 6, 2018

cEP-0018.md Outdated
self.tree = ASTLoader.load_file(file, filename)
self.treeRoot = self.tree
...

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -153,7 +153,6 @@
         self.tree = ASTLoader.load_file(file, filename)
         self.treeRoot = self.tree
     ...
-

Prototype of Py3ASTWalker

cEP-0018.md Outdated
since / and // are different in py3, this is py3 specific
"""
...

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -199,7 +199,6 @@
         since / and // are different in py3, this is py3 specific
         """
     ...
-

Prototype of ASTBear class implementation

cEP-0018.md Outdated
):
"""Actual run method - to be implemented by bear."""
raise NotImplementedError('Needs to be done by bear')

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -240,7 +240,6 @@
             ):
         """Actual run method - to be implemented by bear."""
         raise NotImplementedError('Needs to be done by bear')
-

A test bear

cEP-0018.md Outdated
wherever required
"""
...

This comment has been minimized.

Copy link
@gitmate-bot

gitmate-bot Jun 6, 2018

The text does not comply to the set style.

Origin: MarkdownBear, Section: markdown.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfgdyaw_x/cEP-0018.md
+++ b/tmp/tmpfgdyaw_x/cEP-0018.md
@@ -274,5 +274,4 @@
             wherever required
             """
         ...
-
-```
+```
cEP-0018: Integration of ANTLR in coala
Propose integration of ANTLR in coala.

Closes #118

@virresh virresh force-pushed the virresh:virresh/cEP branch from 861c8eb to 49c6cec Jun 6, 2018

@jayvdb

jayvdb approved these changes Jun 6, 2018

@jayvdb

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

ack 49c6cec

@jayvdb

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

@gitmate-bot

This comment has been minimized.

Copy link

commented Jun 6, 2018

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

@gitmate-bot gitmate-bot merged commit 49c6cec into coala:master Jun 6, 2018

4 checks passed

review/gitmate/commit This commit has no issues. :)
Details
review/gitmate/manual This commit was acknowledged. :)
Details
review/gitmate/manual/pr This PR is reviewed. :)
Details
review/gitmate/pr This PR has no issues. :)
Details
@gitmate-bot

This comment has been minimized.

Copy link

commented Jun 6, 2018

Automated fastforward with GitMate.io was successful! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.