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

Remote templates #36

Merged
merged 6 commits into from
Aug 10, 2019
Merged

Remote templates #36

merged 6 commits into from
Aug 10, 2019

Conversation

ulrikstrid
Copy link
Contributor

No description provided.

src/*
share/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should just remove the share folder probably

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It wouldn't hurt to publish the .re files to npm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can just remove this from the ignore again then. They are not used in this PR anymore but I guess they don't hurt either?

npm-cli/rollup.config.js Show resolved Hide resolved
npm-cli/src/Pesy.re Outdated Show resolved Hide resolved
if (Js.eqNullable(e, Js.Nullable.null)) {
let id = Spinner.start("\x1b[2mRunning\x1b[0m esy pesy\x1b[2m and \x1b[0m building project dependencies");
exec("esy pesy", (e, stdout, stderr) => {
let libKebab = packageNameKebabSansScope;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything from here is basically just refmt.

let id = Spinner.start("\x1b[2mRunning\x1b[0m esy pesy\x1b[2m and \x1b[0m building project dependencies");
exec("esy pesy", (e, stdout, stderr) => {
let libKebab = packageNameKebabSansScope;
let duneProjectFile = Path.(projectPath / "dune-project");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to keep generating a dune-project file and the .opam file, what do you think?

@ulrikstrid ulrikstrid changed the title Initial work on remote templates Remote templates Aug 6, 2019
src/*
share/*
Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It wouldn't hurt to publish the .re files to npm.

);

let libKebab = packageNameKebabSansScope;
let duneProjectFile = Path.(projectPath / "dune-project");
Copy link
Member

Choose a reason for hiding this comment

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

Isn't dune-project file a part of the hello-reason-pesy repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but in this case we're generating a dune-project and package-name.opamif the template doesn't include them for some reason.
But I could just remove these, in that case we could potentially support bucklescript as well if it works with esy in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Well, then this brings up the issue of validation of the templates. For now let's track this in a separate issue and assume the template has all the files needed.

Copy link
Member

Choose a reason for hiding this comment

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

#38

npm-cli/src/Bindings.re Show resolved Hide resolved
@ManasJayanth
Copy link
Member

I guess this will address #5 too

let substituteFileNames = s =>
s
|> Js.String.replaceByRe(
[%bs.re "/__PACKAGE_NAME_FULL__/g"],
Copy link
Member

Choose a reason for hiding this comment

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

Why was this change in the template variable required? (__PACKAGE_NAME_FULL__)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows couldn't handle files named <PACKAGE_NAME_FULL> so this is specific for file names, the in-file replaces are the same as before.

);

let libKebab = packageNameKebabSansScope;
let duneProjectFile = Path.(projectPath / "dune-project");
Copy link
Member

Choose a reason for hiding this comment

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

Well, then this brings up the issue of validation of the templates. For now let's track this in a separate issue and assume the template has all the files needed.

Copy link
Member

@ManasJayanth ManasJayanth left a comment

Choose a reason for hiding this comment

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

I noticed the template variables look different. Was it necessary?

@ulrikstrid
Copy link
Contributor Author

The template variable change is only for the file names, it's the same inside the files.

@ManasJayanth ManasJayanth merged commit f1b0a82 into esy:master Aug 10, 2019
@ManasJayanth ManasJayanth mentioned this pull request Aug 11, 2019
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.

2 participants