Introduce crystal --stdin-filename source flag #4571

akzhan · 2017-06-14T22:37:10Z

to compile source from STDIN, closes #4561.

Synopsys

cat src/math/math.cr | ./bin/crystal build --no-codegen --no-color -o /dev/null -f json --stdin-filename src/math/math.cr

[{"file":"src/math/math.cr","line":6,"column":3,"size":2,"message":"already initialized constant Math::PI"}]

Sija · 2017-06-15T14:28:55Z

src/compiler/crystal/command.cr

+    if is_fake_source_filename
+      sources = [Compiler::Source.new(filenames.first, STDIN.gets_to_end)]
+    else
+      sources = gather_sources(filenames.not_nil!)


.not_nil! ain't needed here, I think.

Thanks, @Sija. Just refactored code, and it's ok.

ysbaddaden · 2017-06-15T14:46:35Z

Bad naming. Without documentation it's impossible to understand what "fake source" means; it's even confusing (why would my source code be fake?).

I'd prefer having - in place of the input filename; it doesn't mean anything by itself (not confusing), and there is history and convention in UNIX tools.

RX14 · 2017-06-15T15:20:45Z

How about --stdin-filename?

akzhan · 2017-06-15T18:06:19Z

--stdin-filepath looks good.

akzhan · 2017-06-15T18:32:16Z

Code refactored, --stdin-filepath now implemented seamlessly.

ysbaddaden · 2017-06-18T21:27:46Z

Urgh. Why not just --stdin ?

RX14 · 2017-06-18T22:01:35Z

@ysbaddaden because it's not very descriptive. I'd assume that --stdin triggers it to read from stdin, and the option would take no arguments. --stdin-filepath is just a lot more descriptive.

akzhan · 2017-06-20T17:36:16Z

@ysbaddaden This is for on-the-fly linters to check source code buffer. Every source file has its path, and it's important for relative "require". Nothing in practice can be built without knowledge of its path.

akzhan · 2017-08-04T14:17:17Z

Any chances to get it merged?)

akzhan · 2017-09-02T08:32:57Z

@RX14 please review this simple feature

Sija · 2017-09-09T10:01:40Z

@RX14 @asterite @mverzilli 🏓

bcardiff

This seems to allow combining both options the stding and additional files. I am not sure there is a use case for that and the order of the scripts might not be clear.

Due to this additional feature (combining stdin and actual files) some additional arrays are created. I think it could be refactored to avoid that.

In case the playground is removed from the compiler this function might come handy.

asterite · 2017-09-18T10:50:29Z

@bcardiff same thoughts as me. This is why I can't approve this. I think we just need one flag without arguments.

akzhan · 2017-09-18T11:18:53Z

@asterite use case - try to compile unsaved code in text editor buffer, which is modified version of on-disk src/tools/something.cr.

There is require "./markup", and compiler should know how to resolve ./markup location.

So we need to provide original/proposed source file name.

asterite · 2017-09-18T11:35:42Z

@akzhan Yes, but it should be:

crystal file1.cr --stdin

That is, --stdin shouldn't expect an argument. The filename is deduced from the usual filename given to the compiler. This is because the compiler can actually receive multiple files:

crystal file1.cr file2.cr --stdin

In this case only the first filename will be used for the output, so the same should applied for the name for stdin.

akzhan · 2017-09-18T11:42:00Z

and which filename is used as proposed one for stdin stream in case of

crystal file1.cr file2.cr --stdin

is stdin file named file1.cr or file2.cr?

--stdin-filepath disallows any ambiguity.

asterite · 2017-09-18T11:49:06Z

@akzhan The first one. I explain it in my comment. If you compile many files, the first one is used for the output name.

Note that in your PR this is accepted:

crystal file1.cr --stdin-file file2.cr

I know this is less ambiguous because you specify the name with --stdin-file, but since the compiler already allows passing multiple files and takes the output name from the first one, doing the same for --stdin is OK (at least consistent). Plus, I don't think in normal cases you will pass more than one file to the compiler with --stdin.

akzhan · 2017-09-18T11:52:26Z

@asterite source file name required not for output (usually editor runs compiler without code generation).

it is required to resolve relative require in source code.

It may be used for output too. But trouble is require relative to file name.

asterite · 2017-09-18T12:13:08Z

@akzhan What do you mean?

akzhan · 2017-09-18T12:30:12Z

I always think about compiling of multiple files and stdin. so I want to inform compiler where is stdin file originated.

Your option - always inform compiler that first file is from stdin.
My option - inform compiler that stdin is that concrete file (not first etc.).

But feel free to apply your option.

asterite · 2017-09-18T13:06:13Z

@akzhan I agree, both options are similar. Maybe your option is better because it's more explicit.

@RX14 Suggested --stdin-filename but you used --stdin-filepath. I think filename is more common.

… STDIN, closes crystal-lang#4561. Synopsys: ```bash cat src/math/math.cr | ./bin/crystal build --no-codegen --no-color -o /dev/null -f json --stdin-filename src/math/math.cr ```

akzhan · 2017-09-18T14:47:41Z

Renamed to --stdin-filename. Added to man page.

Sija · 2017-09-21T23:42:08Z

Time to 🎲

akzhan mentioned this pull request Jun 14, 2017

[WIP] Introduce crystal --fake_source=sourcepath flag #4569

Closed

akzhan changed the title ~~Introduce crystal --fake_source sourcepath flag~~ Introduce crystal --fake-source sourcepath flag Jun 14, 2017

akzhan force-pushed the introduce-crystal-compiler-flag---fake-source-source-path branch from 1bd3250 to f911595 Compare June 14, 2017 22:39

Sija reviewed Jun 15, 2017

View reviewed changes

akzhan force-pushed the introduce-crystal-compiler-flag---fake-source-source-path branch from f911595 to 03476ea Compare June 15, 2017 18:27

akzhan changed the title ~~Introduce crystal --fake-source sourcepath flag~~ Introduce crystal --stdin-filepath sourcepath flag Jun 15, 2017

Sija approved these changes Sep 9, 2017

View reviewed changes

mverzilli approved these changes Sep 18, 2017

View reviewed changes

bcardiff reviewed Sep 18, 2017

View reviewed changes

Introduce crystal --stdin-filename source flag to compile source from…

7085f3c

… STDIN, closes crystal-lang#4561. Synopsys: ```bash cat src/math/math.cr | ./bin/crystal build --no-codegen --no-color -o /dev/null -f json --stdin-filename src/math/math.cr ```

akzhan force-pushed the introduce-crystal-compiler-flag---fake-source-source-path branch from 03476ea to 7085f3c Compare September 18, 2017 14:46

RX14 added kind:feature topic:compiler labels Sep 18, 2017

RX14 approved these changes Sep 18, 2017

View reviewed changes

akzhan changed the title ~~Introduce crystal --stdin-filepath sourcepath flag~~ Introduce crystal --stdin-filename source flag Sep 19, 2017

asterite approved these changes Sep 21, 2017

View reviewed changes

asterite merged commit a214c8d into crystal-lang:master Sep 21, 2017

akzhan deleted the introduce-crystal-compiler-flag---fake-source-source-path branch September 22, 2017 01:12

mverzilli added this to the Next milestone Sep 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce crystal --stdin-filename source flag #4571

Introduce crystal --stdin-filename source flag #4571

akzhan commented Jun 14, 2017 •

edited

Sija Jun 15, 2017 •

edited

akzhan Jun 15, 2017

ysbaddaden commented Jun 15, 2017 •

edited

RX14 commented Jun 15, 2017

akzhan commented Jun 15, 2017 •

edited

akzhan commented Jun 15, 2017

ysbaddaden commented Jun 18, 2017

RX14 commented Jun 18, 2017

akzhan commented Jun 20, 2017 •

edited

akzhan commented Aug 4, 2017 •

edited

akzhan commented Sep 2, 2017

Sija commented Sep 9, 2017

bcardiff left a comment

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017 •

edited

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

Sija commented Sep 21, 2017

Introduce crystal --stdin-filename source flag #4571

Introduce crystal --stdin-filename source flag #4571

Conversation

akzhan commented Jun 14, 2017 • edited

Synopsys

Sija Jun 15, 2017 • edited

Choose a reason for hiding this comment

akzhan Jun 15, 2017

Choose a reason for hiding this comment

ysbaddaden commented Jun 15, 2017 • edited

RX14 commented Jun 15, 2017

akzhan commented Jun 15, 2017 • edited

akzhan commented Jun 15, 2017

ysbaddaden commented Jun 18, 2017

RX14 commented Jun 18, 2017

akzhan commented Jun 20, 2017 • edited

akzhan commented Aug 4, 2017 • edited

akzhan commented Sep 2, 2017

Sija commented Sep 9, 2017

bcardiff left a comment

Choose a reason for hiding this comment

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017 • edited

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

asterite commented Sep 18, 2017

akzhan commented Sep 18, 2017

Sija commented Sep 21, 2017

akzhan commented Jun 14, 2017 •

edited

Sija Jun 15, 2017 •

edited

ysbaddaden commented Jun 15, 2017 •

edited

akzhan commented Jun 15, 2017 •

edited

akzhan commented Jun 20, 2017 •

edited

akzhan commented Aug 4, 2017 •

edited

akzhan commented Sep 18, 2017 •

edited