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

Introduce dynamic manual instructions by value setting from transformation context #233

Open
fabiocarvalho777 opened this issue Aug 16, 2018 · 3 comments

Comments

@fabiocarvalho777
Copy link
Member

fabiocarvalho777 commented Aug 16, 2018

Currently every manual instruction document is static. Allow the usage of transformation context attributes to set portions of manual instruction document via interpolation, by replacing placeholders in the document with values from transformation context attributes.

API changes

Notice the addAtribute method call. The first parameter, "appName", is the placeholder key in the document text, to be replaced by the value of the specified transformation context attribute (the second parameter).

add(new ManualInstruction("Fix invalid Maven dependency versions issues", "MavenEnforcer.md").executeIf(validationFailed).addAttribute("appName", APP_NAME));
@amriteya
Copy link
Contributor

amriteya commented Sep 5, 2018

@fabiocarvalho777 Few observations here.

  • First, I believe we are assuming here that the manual instruction created by one of the preceding TU should already have created a placeholder value. Let me know if that assumption is incorrect.
  • The current implementation dictates that the only way a manual instruction could be added is via appending it to a file (if the file exists or creating a new file and appending data). This needs to be altered based on whether the attribute field is set by addAttribute method.
    For eg in GitHubMdFileManualInstructionsWriter.createManualInstructionDocument, a logic for updating needs to be added with the existing logic of appending.
  • Also, what should be the expected behavior in case the placeholder is not found in the existing manual instruction file? IMO, it should be a soft check with a maximum warning log rather than a drastic step such as halting operation.

Apart from the above points, what is your take on validation of placeholders at the time of creation. From the above requirement, it does not seem like there is a limit on creating a placeholder by TU. This means that there might arise a situation where replication of placeholders could happen. In such a case, what should be the responsibility of addAttribute operation? Should it be intelligent enough to try and resolve which placeholder to replace (IMO, it should not have that complex logic) ?

One more point. How about allowing addAttribute to accept a map rather than a single key, value pair?

Let me know your thoughts on the above-mentioned points.

@fabiocarvalho777
Copy link
Member Author

fabiocarvalho777 commented Sep 5, 2018

@amriteya see my answers below:

First bullet

I believe we are assuming here that the manual instruction created by one of the preceding TU should already have created a placeholder value. Let me know if that assumption is incorrect.

Assuming when you say "manual instruction" you mean the document MD file, that is correct.

Second bullet

Feel free to change the implementation as needed, as long as the overall functionality is preserved and contracts are respected.

Bullet three

During transformation time, validate this way:

  • Attribute not specified: manual instruction TU should result in error.
  • Inexistent attribute with the specified name: manual instruction TU should result in error.
  • Missing placeholder: manual instruction TU should result in warning.

Definition time attributes validation

During definition time, It should check for null and blank attribute names and placeholder names. An IllegalArgumentException should be thrown if validation fails. Use the validation methods already present in TransformationUtility class.

Using a map to specify attributes

I can see that being helpful in very few cases. I believe in most of the cases a regular addAttribute method will be enough, and clearer to read. If we add a map method, would that merge into the existing internal maps or replace it? I am not really sure this is necessary. Maybe we should start simple and not have a map for now. And then, if we realize that would be really helpful, we can add it. Is that ok?

@fabiocarvalho777 fabiocarvalho777 removed this from the 3.0.0 milestone Mar 20, 2020
@Maheswari-Nagendran
Copy link

@fabiocarvalho777 - is there any plan to implement this feature in the near future ?
We are having a scenario to dynamically change something from manual instruction as well as add few text based on transformation step

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants