Contributor guide: implement a new image operation

page version: v2020.12.21-r1

Contributor guide: implement a new image operation

Thank you for taking the time to look into adding a new image operation! I've added some ideas on how to implement it below. If there are questions, always feel free to ask!

The mentioned PR's are suggestions; feel free to use a different amount of PR's, if you believe it could be cleaner.

The suggested implementation guide consists of 3 steps:

• Add the image operation to the operation abstract library (called sic_image_engine)
• Add the operation to the cli_ops parser, which is used by people using cli ops as cli arguments directly (e.g. the cli option sic --blur 1)
• Add the operation to the Pest parser, which is used by people which use the sic script with sic --apply-operations <ops>

PR 1:

• Create an issue for this step, e.g. titled Image Operation engine: add <operation name>

• Implement operation

  • Add an ImgOp for the operation: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_image_engine/src/lib.rs#L16
  • Implement the operation: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_image_engine/src/engine.rs#L110
  • Add tests to verify the operation works as expected. You can use the examples set by other operations. Consider using parameterized tests for similar test cases; the dependency should already exist.

PR 2 part 1:

• Create an issue for this step, e.g. titled Image Operation parsing: add <operation name>

• Consider syntax which works for both cli ops and the --apply-operations script. This means that an operation should be able to be parse-able without introducing ambiguity with both the the Pest parser used for apply-operations script, as well as the cli ops parser used for the cli interface. Additionally, this last syntax should also be compatible with Clap, which is used for the general cli interface (but is inconvenient for the cli ops). The next points will describe the exact steps to implement these parsers. I would recommend doing both in one go to ensure compatibility with each other. You may consider dividing the actual work to multiple PR's, but doing so is optional.

PR 2 part 2:

• Implement cli ops and --apply-operations script parsers

• cli ops:

  • Add an OperationId for the operation: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_cli_ops/src/operations.rs#L18
  • Implement takes_number_of_arguments: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_cli_ops/src/operations.rs#L66
  • Implement create_instruction: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_cli_ops/src/operations.rs#L113
    • To recognise values, we used the ParseInputsFromIter, which will be re-usable when implementing the Pest parser in part 3. Whether you should add a new type and implementation depends on the arguments required for the operation. If the operation arguments are complex, you can use a wrapper (example, instead of an argument tuple or primitive type.

PR 2 part 3:

• --apply-operations script:
  • Add operation to Pest: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_parser/src/grammar.pest#L76
  • Add a Rule for the operation to the Pest PEG file: https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_parser/src/grammar.pest#L36
  • Match the Rule you just made against the Instrs, and transform the Pest Pair(s) to the ImgOp you added at the start of this issue (This ImgOp is wrapped in by the Instr enum): https://github.com/foresterre/sic/blob/33acebfd52b7b8678c433de57513c862695fc30c/components/sic_parser/src/rule_parser.rs#L26. Here you can re-use the value parser used in part 2.
  • Add tests to verify parsing works as expected. You can use the examples set by other operations. Consider using parameterized tests for similar test cases; the dependency should already exist.