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

publish conjure-core cli #21

Merged
merged 3 commits into from
Jun 19, 2018
Merged

publish conjure-core cli #21

merged 3 commits into from
Jun 19, 2018

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Jun 18, 2018

Publish standalone conjure-cli. Stepping stone towards full Java10 cli

@@ -1,5 +1,6 @@
rootProject.name = 'conjure'

include 'conjure'
Copy link
Contributor

Choose a reason for hiding this comment

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

conjure-cli instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

none of the other clis include cli in their name. I'm following that convention, but again its not been hard decided

Copy link
Contributor

Choose a reason for hiding this comment

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

just to clear, so we will have

`conjure` for parser
`conjure-<language>` for language specific generators

can you update the readme to reflect this?


@Value.Immutable
public abstract class CliConfiguration {
abstract Collection<File> inputFiles();
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make this a target?

Copy link
Contributor

Choose a reason for hiding this comment

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

and lazily derived a list of files from the target path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what the value is. We use the input files pretty much immediately

@@ -0,0 +1,63 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

some tests please?

@@ -1,5 +1,6 @@
rootProject.name = 'conjure'

include 'conjure'
Copy link
Contributor

Choose a reason for hiding this comment

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

just to clear, so we will have

`conjure` for parser
`conjure-<language>` for language specific generators

can you update the readme to reflect this?

static CliConfiguration of(String target, String outputDirectory) throws IOException {
File input = new File(target);

Collection<File> inputFiles = ImmutableList.of(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care if input exists?

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 will fail when it tries to parse

if (input.isDirectory()) {
try (Stream<Path> fileStream = Files.find(input.toPath(), 999, (path, bfa) -> bfa.isRegularFile())) {
inputFiles = fileStream
.map(path -> new File(path.toAbsolutePath().toString()))
Copy link
Contributor

Choose a reason for hiding this comment

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

path.toFile()?

apply plugin: 'maven-publish'

distTar.compression = Compression.GZIP
distZip.enabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not was just copied from conjure-java

@ferozco ferozco merged commit 0ec1dcc into develop Jun 19, 2018
@ferozco ferozco deleted the fo/conjure-core-cli branch June 19, 2018 18:02
@iamdanfox
Copy link
Contributor

I think the naming looks good here. Long term, I think it would be kinda cool if people could just brew install conjure and it would pull in this core YML -> IR cli, and a few stock generators. I'd imaging people could then run conjure typescript and it would discover the conjure-typescript executable from the $PATH, maybe even prompt people through some steps (where is your input yml file, where would you like to generate your typescript).

@ferozco
Copy link
Contributor Author

ferozco commented Jun 19, 2018

i like that vision, we should then add a sub-command called compile to only do the YML -> IR step

@iamdanfox
Copy link
Contributor

Especially because this would let people try out conjure in a super user-friendly way, without having to faff with publishing or gradle etc etc. For an MVP, they can just write some yml files, run the CLI to generate some java & typescript and then check in those generated files. I think this would get people off the ground super super fast (even if it has the downsides that it's not as nicely reproducible for other contributors)

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.

3 participants