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

Add new recipe call-graph #5266

Merged
merged 1 commit into from
Feb 18, 2018
Merged

Add new recipe call-graph #5266

merged 1 commit into from
Feb 18, 2018

Conversation

beacoder
Copy link
Contributor

Add new recipe call-graph

Brief summary of what the package does

Library to generate call graph for cpp functions.

Direct link to the package repository

https://github.com/beacoder/call-graph

Your association with the package

maintainer

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

Add new recipe call-graph
@beacoder
Copy link
Contributor Author

@purcell ,
hi, sorry to bother you, but is there anything wrong with my commit, why no-body gives any comments on my commit ?

@purcell
Copy link
Member

purcell commented Jan 30, 2018 via email

@TobiasZawada
Copy link
Contributor

TobiasZawada commented Jan 30, 2018

@purcell Please do not let you push! You are doing a wonderful job with reviewing the melpa packages and it is clear that melpa is not your number one priority -- family as well as your actual job come first -- and code review takes time! Everybody in the queue is politely waiting... But, it is nice to hear that you are still alive;-)).

@beacoder
Copy link
Contributor Author

beacoder commented Jan 31, 2018

@TobiasZawada jesus christ, where did this "still alive" come from. I hope everything is ok with your guys.
@purcell , I'm just curious about why no comments on my commit, no hard feelings, and please do take your time, I'm not in a hurry.
thanks for you guys for doing such a fantastic job for bringing this powerful and convenient tool melpa to make our life a lot easier.

@purcell
Copy link
Member

purcell commented Feb 4, 2018

LOL. :-)

Nice job with this library, @beacoder.

  • Here you should use shell-quote-argument to ensure that the %s will always be safe. Better safe than sorry.
  • You might like to look at the built-in library queue.el. :-)
  • The code to disable linum-mode and nlinum-mode is weirdly specific, and doesn't belong in this file: put it in your own config instead. (What if every major mode tried to predict what would look good to every user?)
  • Setting mode-name is redundant: define-derived-mode generates code to do that for you.
  • Setting list-buffers-directory also looks out of place.
  • You should use kebab-case not camelCase throughout, e.g. here.
  • The window-configuration hack shouldn't be rigidly enforced here: you can use libraries like fullframe in your config to add this behaviour if desired. Just do one thing well!

Hope that helps!

@beacoder
Copy link
Contributor Author

beacoder commented Feb 4, 2018

@purcell, thank you so so so ... much for these great suggestions, I will go through these one by one carefully.

@beacoder
Copy link
Contributor Author

beacoder commented Feb 4, 2018

@purcell , thanks again for your valuable advices, and I have updated code to fix all the issues you mentioned in the previous comment, except this one:
Here you should use shell-quote-argument to ensure that the %s will always be safe. Better safe than sorry.
Since I'm not quite familiar with the usage of shell-quote-argument, could you please be more specific as how should I change the code to use the shell-quote-argument ?
Thanks very much.

@TobiasZawada
Copy link
Contributor

I am sure Steve just meant:

(format "global -a --result=grep -r %s | grep -E \"\\.(cpp|cc):\"" (shell-quote-argument function))

Note that you should allow for the customization of the tagging system and the grep program. Or better just search the result of the tag search by elisp. (Contrary to diff it is very easy to replace grep by elisp.)

@beacoder
Copy link
Contributor Author

beacoder commented Feb 4, 2018

@TobiasZawada ,thanks for answering my question, also I will take your advice into consideration in the future.
@purcell , I have fixed all the issues you mentioned before, could you please help to check again, thanks a lot.

@purcell
Copy link
Member

purcell commented Feb 18, 2018

Thanks for your patience - looks good, so merging now. Welcome to MELPA!

@purcell purcell merged commit a6acf09 into melpa:master Feb 18, 2018
waymondo pushed a commit to waymondo/melpa that referenced this pull request Feb 20, 2018
@beacoder beacoder deleted the call-graph branch February 21, 2018 15:06
@beacoder
Copy link
Contributor Author

@purcell thank you very much.

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.

3 participants