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

Emit directive above header comment (v11) #6172

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

aspeddro
Copy link
Contributor

Close #6170

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Great.
It looks like the test we have directive.res is not affected probably just because of how the compilation of tests happens, without generating header comments.

@cristianoc
Copy link
Collaborator

Indeed this is from test/build.ninja:

bsc_flags = -bs-no-version-header  -bs-cross-module-opt -make-runtime-test -bs-package-output commonjs:jscomp/test  -w -3-6-26-27-29-30-32..40-44-45-52-60-9-106+104 -warn-error A  -I runtime -I $stdlib -I others

Some tests change the bsc flags locally in the file. Let me look up how that's done.

@cristianoc
Copy link
Collaborator

cristianoc commented Apr 18, 2023

Here's an example:

@@config({flags: ["-bs-gentype"]})

@cristianoc
Copy link
Collaborator

One could in principle go here:

    "-bs-no-version-header", set Js_config.no_version_header,
    "*internal*Don't print version header";

and add an option to turn it back on.
Or just don't bother for such a simple test.

@cristianoc
Copy link
Collaborator

Happy to merge as-is, or if you'd like to change something in the tests just lmk.

@aspeddro
Copy link
Contributor Author

aspeddro commented Apr 18, 2023

I can remove the -bs-no-version-header flag on:

bsc_flags = -bs-no-version-header -bs-cross-module-opt -make-runtime-test -bs-package-output commonjs:jscomp/test -w -3-6-26-27-29-30-32..40-44-45-52-60-9-106+104 -warn-error A -I runtime -I $stdlib -I others
In that case all tests will have the header.

@cristianoc
Copy link
Collaborator

I can remove the -bs-no-version-header flag on:

bsc_flags = -bs-no-version-header -bs-cross-module-opt -make-runtime-test -bs-package-output commonjs:jscomp/test -w -3-6-26-27-29-30-32..40-44-45-52-60-9-106+104 -warn-error A -I runtime -I $stdlib -I others

In that case all tests will have the header.

That sounds good to me. No reason why test file should look different from normal compiled files.

@@ -1,5 +1,6 @@
first directive;
second directive;
// Generated by ReScript, PLEASE EDIT WITH CARE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good

@cristianoc cristianoc merged commit d920fb9 into rescript-lang:master Apr 18, 2023
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.

@@directive should be emitted on the first line, above header comment
2 participants