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

jk transform #257

Merged
merged 10 commits into from
Sep 23, 2019
Merged

jk transform #257

merged 10 commits into from
Sep 23, 2019

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Sep 10, 2019

Addresses #253.

  • Add subcommand machinery
  • Supply examples in help text
  • Support literal JavaScript given as the script (-c, --exec)
  • Support a file path given as the script (no flag)
  • Support a module given as the script (-m, --module)
  • Apply transform to each object in multidocs
  • Balk at being expected to print to stdout if formats differ
    • Happily print YAML and YAMLStream (or JSON and JSONStream) to stdout
  • [ ] accept input from stdin (but in which format?)
  • Write files out in output directory
    • (scheme for deciding the paths based on input paths TBD)
  • --inplace (or --overwrite or whatever)
    • balk if it would overwrite a file and --overwrite is not set

@squaremo squaremo force-pushed the cmd/transform branch 3 times, most recently from 673978f to 1788f45 Compare September 14, 2019 18:16
@squaremo
Copy link
Member Author

A fun issue that has come up:

  • part of the mooted design is that --inline (or rather, its absence), will cause any writes that would overwrite a file to be raised as errors. Currently, there's a boolean sent in std.write to say whether an overwrite is OK; if it's false, overwrites will be skipped (i.e., silently failed). One way to get to the mooted design is to have three modes for dealing with (potential) overwrites -- do the overwrite, skip it, or raise an error -- and make these available in the API. Another way would be to keep it as a boolean, but be able to change the meaning of false in the runtime. The latter design may be attractive because it can then be a global flag for the runtime, something like --overwrite=error|skip. (Or further: remove it as an option in the lib, and only control it from the runtime)

@dlespiau
Copy link
Member

For more context, the overwrite fields is/was per-file because of the "node module template" use case: https://github.com/jkcfg/module-template/blob/master/module.js#L201

  • if no README exists, create a basic one
  • if a README already exist, don't touch it

That allows to both: bootstrap a project from scratch and get a full fledged node module ready to commit and regenerate the build files without touching certain files that may have been modified by the user.

The above seems like a nice enough use case to keep /me thinks.

@dlespiau
Copy link
Member

dlespiau commented Sep 16, 2019

This relates somewhat to not writing files if they are identical to what already exists on the fs: #189

@squaremo
Copy link
Member Author

regenerate the build files without touching certain files that may have been modified by the user.

The above seems like a nice enough use case to keep /me thinks.

No argument here; I was hoping to keep all use cases supported as they are now. I think your use indicates a slight preference for controlling the overwrite behaviour entirely from the JavaScript -- what do you reckon?

@dlespiau
Copy link
Member

Yes, I think so too, because it has to be decided by the script author on a per-file basis (in the case of generate). Not sure how this would transpose to transform though, having a global --in-place seems like a good idea too.

@squaremo
Copy link
Member Author

I have realised that it is odd to have --stdout and for the default to be true -- that means, whenever you want to write to files, you pass --stdout=false. I think it might be better to default to be consistent with generate by outputting to files unless --stdout; and to default to raising an error before overwriting a file (ideally, before any files are written).

@squaremo
Copy link
Member Author

Re --stdin: accepting values from stdin is tricky because we don't know what format it's in. We could have --stdin=yaml perhaps. Rather than making a decision, I'm punting it into the future.

@squaremo
Copy link
Member Author

I think it might be better to default to be consistent with generate by outputting to files unless --stdout; and to default to raising an error before overwriting a file (ideally, before any files are written).

@dlespiau I also think that latter (raise an error rather than overwriting a file) would be a good default for jk generate -- I can do this in another PR, if you agree ..

This adds a no-op `transform` command, and factors out some
commonality with the run command.
This gives `jk transform` an implementation which corresponds to

    jk transform --stdout -c <expr> <file>...

It's the simplest thing to get working, because it's the most like
previous commands.
This implements the `-m` (resolve as a module) flag, and the no flag
(resolve as a filesystem path) case.

This is similar to `-c` (literal JavaScript), except that the supplied
argument is used as an import, rather than the body of a function. I
decided it would be easier to use two different templates --
internal/transform-module.js and internal/transform-exec.js -- rather
than one. But this approach does mean the core bit needs to be in a
public module (transform.js).

Though I'd factored out the code for finding out an appropriate base
path for resolving things (establishScriptDir), it turns out in this
case the appropriate base path is the current directory. As with `jk
generate`, the path or module is imported from a _temporary module_,
and unless its own path is the current directory, the resolution won't
work.
This makes `jk transform` transparently apply its transformation to
all the objects in a YAML or JSON multidoc.

I realised that once the inputs are transformed, we are essentially in
the same situation as `jk generate` -- we have a bunch of objects that
need to be output, and hints as to their format. For that reason, I've
factored that out of the JavaScript side of generate, so it can be
called from transform.
 - pass `overwrite` flag through to generate.js (NB this changes `jk
   generate`, by changing the default of overwrite from true to false)
 - add a test for writing to files
Previously the choice for a write that would overwrite a file was to
skip it, or overwrite the file. For `jk transform` (and possibly other
things), the ideal is to throw an error unless it's been explicitly
allowed.

This commit makes the overwrite field an enum, and changes the runtime
protocol to return an error if that overwrite option is used. A
boolean is also accepted in the JS API, for backward compatibility.
If the input files (and therefore the inferred output files) are
different formats, then it's not possible to output them as a stream
to stdout. Since `jk transform` delegates the output bit to generate,
it carries over this prohibition; this adds a test to make sure it
remains so.
Rather than having --stdout=true be the default, meaning you need to
supply --stdout=false if you want to write to files, it is more
consistent with `jk generate` to assume file output.

To keep it safe and clean, the default is _also_ to raise an error
rather than overwriting.
@squaremo squaremo marked this pull request as ready for review September 18, 2019 15:25
@squaremo
Copy link
Member Author

$ jk transform --help
Transform configuration files

Usage:
  jk transform <script> <file>... [flags]

Examples:

  running the default export of a module (or file) on each input document
    jk transform -o outputdir/ ./script.js ./inputdir/*.json
  running a function on each input, and printing the results to stdout
    jk transform --stdout -c '({ name: n, ...fields }) => ({ name: n + "dev", ...fields })' inputdir/*.yaml


Flags:
  -d, --emit-dependencies         emit script dependencies
  -c, --exec                      treat first argument as specifying literal JavaScript to execute
  -h, --help                      help for transform
  -i, --input-directory string    where to find files read in the script; if not set, the directory containing the script is used
  -m, --module                    treat first argument as specifying a module to load
  -o, --output-directory string   where to output generated files
      --overwrite                 allow input file(s) to be overwritten by output file(s); otherwise, an error will be thrown
  -p, --parameter name=value      set input parameters
  -f, --parameters filename       load parameters from a JSON or YAML file
      --stdout                    print the resulting values to stdout
  -v, --verbose                   verbose output

Wrinkle: --input-directory is a global flag, but doesn't have an interpretation here (because the individual files are named explicitly as arguments).

The flag --input-directory has no meaning for `jk transform`, because
its arguments explicitly name the files to run through the transform.

This commit splits the global flag initialisation into two bits, so
that the transform command can omit that particular flag.
@squaremo squaremo changed the title Add transform command jk transform Sep 19, 2019
@squaremo squaremo merged commit 391d562 into master Sep 23, 2019
@squaremo squaremo deleted the cmd/transform branch September 23, 2019 09:36
@dlespiau
Copy link
Member

oooh, exciting, can't wait to play with it!

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