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

Make crystal tool expand work on project files. #18

Closed
wants to merge 3 commits into from

Conversation

yxhuvud
Copy link
Contributor

@yxhuvud yxhuvud commented Mar 25, 2018

I did a quick attempt at the flycheck mode too (which would need a very similar fix), but didn't manage to do it the seemingly obvious way.

It would have been nice to be able to get a proper list of compile targets somehow :/

EDIT: Figured out how to make flycheck happy.

@RX14
Copy link
Contributor

RX14 commented Mar 25, 2018

Perhaps the target files from shard.yml? Sounds like a pain to parse though.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Mar 26, 2018

@RX14 Hmm. Interesting. Sadly, there doesn't seem to exist a well maintained yaml parser for elisp. The only I've found is a libyaml binding that doesn't seem to be maintained, and that isn't packaged.

It seems targets is only inserted into shard.yml when using crystal init app foo, not when doing crystal init app foo, so I am not certain that is a sufficient solution.

@RX14
Copy link
Contributor

RX14 commented Mar 26, 2018

Starting from a spec suite file is probably more robust.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Mar 26, 2018

Like spec_helper.cr? Yeah, perhaps. I guess adding those that exists of spec_helper.cr and {{project-name}}.cr would be possible.

I think I figured out the flycheck options, but I noticed that the show implementation doesn't work properly - it doesn't crash as before, it just can't find any implementation or call.

@RX14
Copy link
Contributor

RX14 commented Mar 26, 2018

Nah, any old **/*_spec.cr file is fine.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Mar 26, 2018

I'm afraid I don't see the logic behind that. Either there is some central file that requires everything, and then spec_helper.cr is most likely the one by a large margin, or they don't and it doesn't help.

@RX14
Copy link
Contributor

RX14 commented Mar 26, 2018

@yxhuvud yeah fair enough, I was working under the assumption that each spec file required the whole application which is wrong...

@brantou
Copy link
Member

brantou commented Mar 27, 2018

@yxhuvud Although there is no good yaml parser available in elisp, crystal's YAML module can be used.

crystal-mode.el Outdated
@@ -2757,5 +2767,6 @@ directory of the current file."
(add-to-list 'interpreter-mode-alist (cons (purecopy name) 'crystal-mode)))

(provide 'crystal-mode)
(provide 'crystal-tool--project-name)
Copy link
Member

Choose a reason for hiding this comment

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

Function export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan was to not have to have the same function defined in both this and the flycheck file. Perhaps there is a better way to do it?

Copy link
Member

@brantou brantou Mar 27, 2018

Choose a reason for hiding this comment

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

@yxhuvud provide and require are an alternative to autoload for loading files automatically. They work in terms of named features. Autoloading is triggered by calling a specific function, but a feature is loaded the first time another program asks for it by name.
https://www.gnu.org/software/emacs/manual/html_node/elisp/Named-Features.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I had some issues before but they don't seem to happen anymore so I removed it and the require statement as the latter seems to break byte compilation for me.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Apr 15, 2018

@brantou BTW, how would I do that? While crystal run filename [..] may work, that requires an actual file to compile, and I'm not certain how to supply one from STDIN in a portable way, meaning it would have to either generate it on demand or generate and compile on demand, or something. Thoughts?

@RX14
Copy link
Contributor

RX14 commented Apr 15, 2018

@yxhuvud crystal run --stdin-filename /path/to/usaved/file.cr and pass the file to stdin. The crystal compiler can read files from stdin, but it needs to have a "fake" (or real) path for where the file actually is.

@brantou
Copy link
Member

brantou commented Apr 19, 2018

@yxhuvud The code that parses yaml must be very short, it can be stored directly as a string, and written to a temporary file when executed. Then use call-process to call crystal run tmpfile.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Apr 29, 2018

So I finally got the time to investigate a bit, and running
time crystal run parse.cr
with parse.cr being:

require "yaml"

yml = YAML.parse(File.read "shard.yml")
p yml

outputs

{"name" => "Cabbage",
 "version" => "0.1.0",
 "authors" => ["Linus Sellberg <sellberg@gmail.com>"],
 "license" => "ICS"}

real	0m0,865s
user	0m0,975s
sys	0m0,316s

crystal run with an empty file as argument still takes 0.6s.

I feel the added delay makes that path to improve this a nonstarter.

(A precompiled version takes a much more reasonable 0.004s)

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Apr 29, 2018

Additionally, I tried to add the spec/spec_helper.cr, but then I don't get any expansions at all.

This do a best effort solution for the case where a project has a root
file that require all other files, and then a macro is expanded in a
project file that make use of something that is defined in a different
file.

Is it a perfect solution? No, especially not for app projects having
multiple compile targets. But it works for libraries created with
"crystal init lib".
@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 10, 2019

(updated the flyckeck file to handle the conflict that had arisen against the master branch).

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

Successfully merging this pull request may close these issues.

5 participants