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

Parse from stdin #49

Closed
smeevil opened this issue Mar 3, 2016 · 8 comments
Closed

Parse from stdin #49

smeevil opened this issue Mar 3, 2016 · 8 comments
Assignees

Comments

@smeevil
Copy link
Contributor

smeevil commented Mar 3, 2016

Hi,

I would like to suggest to allow Credo to parse source from stdin.
The reasoning behind this is that it would make it easier for editors to make use of Credo.
For example with Spacemacs and Flycheck.

I know its possible to use it with Flycheck at the moment, but it requires the file to be saved. If you use continues flychecking it will thus also continuous safe the file either in the original or make a temp file to pass along to Credo to let it do its job.

If we can pass the buffer's contents from Spacemacs directly to Credo via stdin, we can bypass the saving/creation of temp files.

I've already create a pull request supporting this behaviour where you can use it as follows.

Just passing in source via stdin :

$ echo 'IO.puts( "hello world");' |  mix credo --format=flycheck --stdin
# stdin:1: C: There is no whitespace around parentheses/brackets most of the time, but here there is.

Passing in source via stdin and using a filepath to map:

$ echo 'IO.puts( "hello world");' |  mix credo --format=flycheck --stdin /path/representing/the_current/source.ex
# /path/representing/the_current/source.ex:1: C: There is no whitespace around parentheses/brackets most of the time, but here there is.

Do note with the last option passed as filename is a stub that is just used to prefix the error so certain editors can annotate the original file.

You can find the pull request here : #50

@rrrene
Copy link
Owner

rrrene commented Mar 3, 2016

First let me thank you for taking the time to put this together. Using STDIN for this is certainly something useful.

so certain editors can annotate the original file

Which editors are these? I am asking because the hack in the PR to make this work is really something I would like to avoid.

@rrrene rrrene self-assigned this Mar 3, 2016
@smeevil
Copy link
Contributor Author

smeevil commented Mar 4, 2016

An example of Spacemacs using it with Flycheck and realtime updating :

image

@smeevil
Copy link
Contributor Author

smeevil commented Mar 4, 2016

Just to be clear, which hack are you referring to, the one that prefixes the filename stub ?

@rrrene
Copy link
Owner

rrrene commented Mar 4, 2016

Maybe "hack" was a bit harsh. Sorry, it was late and was a bit grumpy. 😞

To clarify, please take a look at this branch, where I merged your commits and made some changes to better fit the "bigger picture":

  1. The hack I was reffering to was setting the files field in the Config struct during CLI parameter parsing. The module responsible for converting the parameters to source file locations is Credo.Sources. Separating these concerns makes it easier to reason about the system as a whole.
  2. Setting the source files in the Suggest command via a new function get_source_files violated this. And we have 3 commands (suggest, list and explain) that need to be able to get sources from STDIN.
  3. As a general guideline: When contributing to a project, always try to adhere to already present styles, formatting rules etc. This makes it much liklier that a PR gets merged, since the original author does not "trip over" cosmetic inconsistencies when reviewing changes to actual functionality.

That said, I personally hate it when people are super nitpicky about PRs and ask you to modify white-space or other trivial parts of your commits. Therefore I merged the changes into the above mentioned branch and will merge that integration branch into master soon!

Your input to this project was already very valuable and I sincerely hope that the above "lecture" does not stop you from contributing more 👍

@smeevil
Copy link
Contributor Author

smeevil commented Mar 4, 2016

No worries :D

yeah, I agree it was a bit invasive to set the files option like that but had no other idea at the moment on how to accomplish it otherwise.

I really like the way you handle this. There are indeed a lot of people that are very picky about their pull requests which, at least for me, takes away all the enthusiasm and excitement to help with those projects.

For me personally a pull request is more a prove of concept. I've had an idea which might be really useful for others as well which i'll try to incorporate as best as i can with the limited knowledge of the existing code base, but when you are new to a piece of code, you have no idea what the details are. In that case i'll try to prove my solution and how it works as best as i can. For the maintainer it should be quite clear what the proposed suggestion and result should be, but as he/she does have a mental map of the original code base, its blatantly clear that the new code is invasive and can be handled in a much nicer way. With the given information he/she should be able to quickly refactor it and incorporate the new functionality, as you did :)

In the end, all that matters to me, is that the intended behaviour is adopted and working, not necessarily by using my code verbatim :)

I just started out with Elixir about 3 months ago and switched to Spacemacs about two weeks ago. The switching to Spacemacs (and with it, coincidentally, evil-mode, which i should have done like 20 years ago...but thats besides the point) opens up a lot of new experiences and opportunities to spot missing behaviour or tools and lear about new ones. This prompted me to try and incorporate Credo into Flycheck :)

Anyways,
I'll be around and keep an eye on the issue tracker to see if i can help in any way :)

Have a wonderful day !

Gerard.

@rrrene
Copy link
Owner

rrrene commented Mar 11, 2016

Hi Gerard,

your changes are part of the latest release - v0.3.6.

Please check if the "STDIN feature" now works to your content. Please also take note that the CLI switch was renamed to --read-from-stdin which is a notch more descriptive.

btw: are you by any chance attending this year's ElixirConf.EU?

@smeevil
Copy link
Contributor Author

smeevil commented Mar 11, 2016

Hi,

Great to hear , will let you know if it works.

Regarding elixir conf, would have loved to, but a day later is WGT festival in Leipzig. This is something I always look forward to and have been going there every year since the last 15 years :) (http://www.wave-gotik-treffen.de/english/bands.php).

Let me know if you are ever in NL though so we can catch some beers :)

On 11 Mar 2016, at 23:08, René Föhring notifications@github.com wrote:

Hi Gerard,

your changes are part of the latest release - v0.3.6.

Please check if the "STDIN feature" now works to your content. Please also take note that the CLI switch was renamed to --read-from-stdin which is a notch more descriptive.

btw: are you by any chance attending this year's ElixirConf.EU?


Reply to this email directly or view it on GitHub.

@smeevil
Copy link
Contributor Author

smeevil commented Mar 29, 2016

Sorry for the late reply :/
I tested it out and everything seems to work perfectly :)

Thank you so much :)

Gerard.

@smeevil smeevil closed this as completed Mar 29, 2016
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

No branches or pull requests

2 participants