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

PowerShell: add tag generation from class #3476

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

kumarstack55
Copy link
Contributor

This pull request adds functionality to the existing parser.

Previously, the PowerShell parser generated tags from functions and variables.

By merging this pull request, it will be able to generate tags from classes in addition to those.

Please note that I do not have a lot of experience contributing to the open source community. Also, this is my first time attempting to contribute to u-ctags.

Please let me know if there are any details I should address.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #3476 (d45a7a4) into master (096dc1c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d45a7a4 differs from pull request most recent head c594496. Consider uploading reports for the commit c594496 to get more accurate results

@@           Coverage Diff           @@
##           master    #3476   +/-   ##
=======================================
  Coverage   83.33%   83.34%           
=======================================
  Files         219      219           
  Lines       52761    52789   +28     
=======================================
+ Hits        43967    43995   +28     
  Misses       8794     8794           
Impacted Files Coverage Δ
parsers/powershell.c 91.16% <100.00%> (+0.96%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

parsers/powershell.c Outdated Show resolved Hide resolved
parsers/powershell.c Outdated Show resolved Hide resolved
parsers/powershell.c Outdated Show resolved Hide resolved
@masatake
Copy link
Member

masatake commented Sep 3, 2022

Please note that I do not have a lot of experience contributing to the open source community. Also, this is my first time attempting to contribute to u-ctags.

I'm proud of u-ctags receiving a contribution from a person like you.

I gave some comments. Feel free to do git commit --amend and git push --force.

If you are interested in improving the PowerShell parser, I have more comments.

  • filling inherits: field

    MyException	input.ps1	/^class MyException : Exception {$/;"	c	inherits:Exception
    
  • using keyword API
    You extended the types of token: KEYWORD_FUNCTION, and KEYWORD_CLASS.
    However, keeping the type KEYWORD but adding keyword field to tokenInfo.

@kumarstack55
Copy link
Contributor Author

If you are interested in improving the PowerShell parser, I have more comments.

  • filling inherits: field
MyException	input.ps1	/^class MyException : Exception {$/;"	c	inherits:Exception

This improvement would be better.
However, I do not intend to make this improvement in this PR.

This is because I cannot think of a good way to do it right now, because I do not know how it should work with multiple inheritance and interface implementations, and how it should be implemented.

@kumarstack55
Copy link
Contributor Author

  • using keyword API

You extended the types of token: KEYWORD_FUNCTION, and KEYWORD_CLASS.
However, keeping the type KEYWORD but adding keyword field to tokenInfo.

Thank you for your review.
However, I feel I still do not understand your comment.
Could you please comment so that I may understand.

My current PR is as follows:

  • (a) split TOKEN_KEYWORD into TOKEN_KEYWORD_FUNCTION, TOKEN_KEYWORD_CLASS, and TOKEN_KEYWORD_OTHER. The token->type attribute determines whether the keyword is a function or a class.
  • (b) do not change tokenInfo.

Is the proposed implementation of @masatake as follows. :

  • (a) do not split TOKEN_KEYWORD.
  • (b) add an attribute like token->keywordType to tokenInfo. When token->type is TOKEN_KEYWORD, token->keywordType attribute determines if the keyword is a function or a class.

@masatake : Is this understanding correct?

@masatake
Copy link
Member

masatake commented Sep 4, 2022

Your understanding is correct.

However, my proposal about keyword may make this pull request too bigger.

So about keyword, I propose the following change in this pull request for keeping this pull request small and simple.

  • Remove TOKEN_KEYWORD_OTHER IF there is no trouble.
    (Introducing TOKEN_KEYWORD_FUNCTION and TOKEN_KEYWORD_CLASS is acceptable.)

Let's forget token->keywordType

@kumarstack55
Copy link
Contributor Author

Thank you for your comments.

I have finished making changes to the code.
@masatake : Could you please confirm?

If everything is ok, I will squash the commits.
The squash should be the following two commits:

  • PowerShell: add tag generation from class
  • PowerShell: remove unnecessary if statements

@masatake
Copy link
Member

masatake commented Sep 4, 2022

@masatake : Could you please confirm?

Fine. Please squash the commits into the two.

@kumarstack55
Copy link
Contributor Author

I squashed it.
I would appreciate it if you could merge.

@masatake masatake merged commit a5721f6 into universal-ctags:master Sep 5, 2022
@masatake
Copy link
Member

masatake commented Sep 5, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants