Skip to content

Improved Elixir optlib parser - #2024

Closed
dreamtigers wants to merge 12 commits into
universal-ctags:masterfrom
dreamtigers:elixir-optlib
Closed

Improved Elixir optlib parser#2024
dreamtigers wants to merge 12 commits into
universal-ctags:masterfrom
dreamtigers:elixir-optlib

Conversation

@dreamtigers

@dreamtigers dreamtigers commented Mar 3, 2019

Copy link
Copy Markdown
Contributor
  • Took the branch elixir-optlib and rebased it to a most recent master.
  • Added some spaces in optlib2c for a better looking C code.
  • Improved the Elixir parser elixir.ctags (sent a pull request to the upstream but made the same improvements here, in fewer commits for conciseness)
  • Added a more exhaustive test case for the new parser, as requested in Elixir: new parser #1806.

masatake and others added 5 commits March 2, 2019 06:05
@masatake @dreamtigers
Elixir: new parser
adf6cdc 
Imported from https://github.com/mmorearty/elixir-ctags.
Close #1758.
See also mmorearty/elixir-ctags#5.

The change from the original .ctags.

* Put the original LICENSE file as the header of the .ctags.
* Use --map= option instead of --langmap because optlib2c doesn't handle
  --langmap option.
* Define kinds explicitly with --kinddef-<LANG> option.
* Remove backslashes before double quotes chars in the regex pattern for
  "test".
* Use singular forms for kind names.

TODO: Test cases for above kinds are needed.

    c  callbacks (defcallback ...)
    d  delegates (defdelegate ...)
    e  exceptions (defexception ...)
    i  implementations (defimpl ...)
    a  macros (defmacro ...)
    o  operators (e.g. "defmacro a <<< b")
    p  protocols (defprotocol...)
    r  records (defrecord...)

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@dreamtigers
optlib2c: Change some indentation for aesthetics purposes
ffd1e98
@dreamtigers
Update elixir.c to lastest optlib2c
e06ed3a
@dreamtigers
Add a more exhaustive Elixir test case
37bb27e
@dreamtigers
Improve the Elixir parser
1ad9b8d 
I already made a pull request with *detailed commits* to the upstream of
the elixir.ctags parser. Said pull request and commits can be found in
mmorearty/elixir-ctags#8

This parser is missing basically two things which are annotated in the
input.ex test with a TODO keyword, plus an explanation of _why_ they're
missing.  Said missing features are listed bellow:

* exceptions: I don't know how to identificate or differenciate
exceptions from one another because they do not have a name. Instead,
they resemble an struct. For example, if I have:

```
defmodule MyAppError1 do
  defexception [:message]
  # code
end

defmodule MyAppError2 do
  defexception [:message]
  # code
end
```

How would/should ctags differenciate between these two exceptions?

* word-defined logical operators (`and, or, not`): The elixir parser in
this commit already has the regex atom including these logical
operators, but since the regex engine used by universal-ctags does not
have lookahead, I couldn't think of a way to add them or negate them.

The test for them is commented in, for the time someone is brave enough
to add them.
masatake
masatake reviewed Mar 3, 2019
View reviewed changes
Comment thread makefiles/optlib2c_input.mak
optlib/ctags-optlib.ctags \
optlib/elm.ctags \
optlib/elixir.ctags \
optlib/elm.ctags \

@masatake masatake Mar 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not touch elm.ctags.

@dreamtigers dreamtigers Mar 4, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was a conflict when rebasing 09861b6 to master.
The tabs in the line optlib/elm.ctags were corrected in 806f296, but that commit happened after 09861b6, which triggered the conflict.

I should have seen and corrected the tabs, tho.

@dreamtigers dreamtigers Mar 4, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trying out Github's suggestions feature, sorry if it's messy)

Suggested change
optlib/elm.ctags \
optlib/elixir.ctags \
optlib/elm.ctags \

Comment thread misc/optlib2c
Comment thread Units/simple-elixir.d/input.ex Outdated
Comment thread Units/simple-elixir.d/input.ex Outdated
@masatake

masatake commented Mar 3, 2019

Copy link
Copy Markdown
Member

You can run the test with "make units LANGUAGES=Elixir" locally.

@coveralls

coveralls commented Mar 3, 2019 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.006%) to 85.243% when pulling 750386c on dreamtigers:elixir-optlib into 16718d4 on universal-ctags:master.

@masatake

masatake commented Mar 3, 2019

Copy link
Copy Markdown
Member

Could you consider a ".b" test case for the TODO items?
See .b test in "Gathering test cases for known bugs" in http://docs.ctags.io/en/latest/units.html.

Some valuable studies about changing the regex engine used in u-ctags are in #1861.

@dreamtigers
Move elixir's test to a category and divide to smaller cases
c0aca49
@dreamtigers
Add two test cases with bugs
eff912a 
More info on the bugs can be found in the test cases' `README.md`.
@dreamtigers
Improve regex for tests and remove operators comment
88a6546 
Moved that comment to the README.md of a bugged test case in the Elixir
directory.
@dreamtigers
Add 'exclusive' tag to operator regex
ef545ae 
Fixes the bug where the parser wouldn't correctly generate tags for the
'word' operators (and, or, not, etc...)
@dreamtigers
Move test case from a bugged one to a correct one
750386c
@dreamtigers

dreamtigers commented Mar 7, 2019

Copy link
Copy Markdown
Contributor Author

I renamed the simple-elixir.d directory to parser-elixir.r. Divided the test case in smaller cases, so it was worth it to rename the directory (as specified in docs/units.rst, in the section 'Categories'). And fixed one of the TODO's where the parser wasn't generating the right tags for the word logical operators (and,or,not, etc...).

masatake
masatake reviewed Mar 7, 2019
View reviewed changes
Comment thread docs/news.rst
* Diff
* DTD
* DTS
* Elixir *optlib*

@masatake masatake Mar 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.

@masatake

masatake commented Mar 7, 2019

Copy link
Copy Markdown
Member

Thank you. I will take over this. I will merge this after squashing some commits to keep change history simple. I'm in busy now. So, it will take few days.

@masatake masatake mentioned this pull request Mar 7, 2019
Merged
masatake added a commit that referenced this pull request Mar 7, 2019
@masatake
Merge pull request #2034 from masatake/dreamtigers-elixir-optlib+masa…
6365358 
…take

Elixir: rearranged-before-merge PR for #2024
@masatake

masatake commented Mar 7, 2019

Copy link
Copy Markdown
Member

Merged after rearranging the commits posted this PR. See #2034.
Thank you.

@masatake masatake closed this Mar 7, 2019
@dreamtigers
dreamtigers deleted the elixir-optlib branch March 12, 2019 19:00
@dreamtigers

dreamtigers commented Mar 12, 2019

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the support and improving the parser! Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@masatake masatake masatake left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dreamtigers @masatake @coveralls