Add methods for work with yaml file #6769

antarus · 2017-11-29T18:16:09Z

This code allows you to add one or more properties to a yaml file.
code update from hipster-labs/generator-jhipster-spring-cloud-stream

Please make sure the below checklist is followed for Pull Requests.
Travis tests are green
Tests are added where necessary
Documentation is added/updated where necessary
Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

code from hipster-labs/generator-jhipster-spring-cloud-stream

correct unit test on windows correct unit test for windows user

deepu105

What is the need for these functions? I'm not in favour of dumping these here without a solid reason. They seem to be very specific for YML handling in that case why not just publish it as a standalone open-source library and use it where needed. Atleast I dont see any need for these within the generator

deepu105 · 2017-11-29T21:39:02Z

generators/utils.js

+ * @param {string} generator - The generator
+ * @return {string} the value of property or undefined
+ */
+function getPropertyInArray(array, name, generator) {


Please use lodash directly. This method doesnt add any value so please remove it

deepu105 · 2017-11-29T21:39:15Z

generators/utils.js

+ * @param {string} generator - The generator
+ * @param {string} value - (optional) value of the property
+ */
+function updatePropertyInArray(array, name, generator, value) {


Please use lodash directly. This method doesnt add any value so please remove it

deepu105 · 2017-11-29T21:39:22Z

generators/utils.js

+ * @param {string} name - property name to search "format myProperty.level2.level3"
+ * @param {string} generator - The generator
+ */
+function deletePropertyInArray(array, name, generator) {


Please use lodash directly. This method doesnt add any value so please remove it

deepu105 · 2017-11-29T21:42:06Z

generators/utils.js

+ * @return {string} the value of property or undefined
+ */
+function getYamlProperty(file, name, generator) {
+    const treeData = jsyaml.safeLoad(generator.fs.read(file));


This is not efficient if reading multiple props. Ideally, you should just load file once and read all required values from that. Its better to do this directly using the library. I dont see added value in having this method

deepu105 · 2017-11-29T21:42:53Z

generators/utils.js

+ * @param {array} properties - array of property to add
+ * @param {string} generator - The generator
+ */
+function addYamlPropertiesAtBeginin(file, properties, generator) {


method name should be addYamlPropertiesAtBeginning

deepu105 · 2017-11-29T21:49:40Z

generators/utils.js

+ * @param {string} numberSpace - number espace to start
+ */
+function addYamlPropertiesAtLineIndex(file, properties, generator, indexLine, numberSpace) {
+    const body = generator.fs.read(file);


pass body, dont read

deepu105 · 2017-11-29T21:49:51Z

generators/utils.js

+        spaceStr += ' ';
+    }
+    bodyLines.splice(indexLine, 0, newLines.map(line => spaceStr + line).join('\n'));
+    generator.fs.write(file, bodyLines.join('\n'));


return result, dont write

deepu105 · 2017-11-29T21:51:19Z

generators/utils.js

+
+/**
+ * Add a yaml property at beginin
+ * TODO manage value of array type


please don't add incomplete methods to public util file

deepu105 · 2017-11-29T21:52:27Z

generators/utils.js

+}
+
+/**
+ * Get the last property of a common hierarchical.


write a better explanation comment

deepu105 · 2017-11-29T21:56:14Z

generators/utils.js

+}
+
+/**
+ * Retourne l'index de la ligne d'une propriété simple et unique


please write comments in english

deepu105 · 2017-11-29T22:01:15Z

@antarus I don't think these should be added to the main generator. They are too specific to handle YML files. Maybe just publish them as a generic utility library

antarus · 2017-11-30T08:41:18Z

Hello, the goal is to facilitate the integration of external module to the main generator. There is currently nothing in the generator to add, edit a spring's configuration file . For me, it's missing. There are function to add graddle or maven dependencies, for me it's the same notion. It took me a long time to manage this for the stream-cloud module.
I haven't found any library that allows you to add a yaml property without removing all the formatting, existing comments, etc. (I may not have searched well!)

I think the main generator lacks tools to make life easier for module developers and their integration.

After about the code deportation in an external library, I don't know, I'm not a node developer, I've never done it.

sendilkumarn · 2017-11-30T08:43:58Z

I was thinking about something similar for JHipster Modules (while working on JHipster-kotlin)🤔 where we need to add few things extra to the base build.gradle or pom.xml something like this will be really useful. And again these should be a separate library.

something similar to angular-cli, how it is generating the application

deepu105 · 2017-11-30T10:27:44Z

@antarus let me think about this. May be I can create a utils project under jhipster and you can PR these there. Give me some time

pascalgrimaud · 2017-11-30T10:57:27Z

As @antarus said, it's for adding properties into spring boot yaml configuration. For that, currently, we have differents solutions:

add at the end of yaml file the full key/value, like: hello.world.name.properties: value. It's not consistent with the other properties but it works
add properties into a new file: application-yolo.yml and say to the user to start the project by adding the profile yolo
try to regexp / add / replace the yaml properties
- every module can copy/paste these functions if needed -> it's in spring-cloud-stream module today
- integrate these functions written by @antarus, into main generator
- integrate these functions to an existing library -> it's strange that it doesn't exist today. Maybe pull request to one of existing library ?
- made a new one

deepu105 · 2017-11-30T21:24:09Z

@pascalgrimaud yes I understand the need but I feel that if we start adding everything into the main generator it will become like a kitchen sink. Let's do a new library called jhipster-utils for these kinds of stuff. I'll create the repo and publish a skeleton and then @antarus can do the PR there

PierreBesson · 2017-11-30T21:57:30Z

For me this is a valid use case. There should be an API to add properties from a module. However manipulating YAML files sucks, so I'm a bit reserved on this.

@deepu105, Do we really need to create yet another repo and npm package for which we will need to manage the release cycle. I have already lost track of how many we already have !

deepu105 · 2017-11-30T22:25:40Z

@PierreBesson multiple npm repo are better than making our core a kitchen sink. Imagine the amount of code that would need to be updated each time we rewrite something, say for example like the ES6 update I did. These are too specific functions for YML handling which ideally should be done by a lib like jsyaml, but if not either PR to that or worst case create a library to do that. This lib might never need much maintenance. Given the current size of Jhipster codebase, I'm reserved on adding stuff like these here. If is is one or two methods that's fine but this is too much code that i'm not willing to maintain in core. Remember every line of code we add makes our life tougher 😉

deepu105 · 2017-11-30T22:27:59Z

Also the main problem is that these methods are never used by the generator itself its only used by one module now and if another module needs it in the future than an external lib is ideal for that

PierreBesson · 2017-11-30T22:39:58Z

@deepu your 2nd point convinced me. Still wished we didn't have so many repos but I guess this can't be helped.

pascalgrimaud · 2017-12-01T01:02:55Z

Agree with you @deepu105
That's why my prefered solution for this need is:

add at the end of yaml file the full key/value, like: hello.world.name.properties: value. It's not consistent with the other properties but it works

deepu105 · 2017-12-01T08:02:22Z

@pascalgrimaud I dont understand one thing cant you load the YML using jsyaml and add new props and stuff to the parsed object and then dump the object back using jsyaml? Isnt that what we do in our Kubernetes generator and stuff? why is it required to do all these manual parsing? Sorry of its a stupid question

pascalgrimaud · 2017-12-01T08:05:54Z

@antarus already tried this, it works fine but we lost all comments, which are so important

antarus · 2017-12-01T08:12:55Z

Hello.

there are two problems with jsyaml, the first like the pascal says, we lose all the comments.

the second problem, when we load the application-dev file and save it, it adds 'null' to the 'application:' property

deepu105 · 2017-12-01T08:14:40Z

@pascalgrimaud @antarus ok I understand. Did we raise the issues to jsyaml guys? I'm pretty sure they might be able to fix them since its a valid usecase?

deepu105 · 2017-12-01T08:15:16Z

Since no other repo uses these at the moment if jsyaml guys can fix the issue I guess that would be the best solution

antarus · 2017-12-01T08:24:26Z

jsyaml has not been updated since September 19th, the null is report since January 24th, and the comments not manage it seems to me that it's also reported ..
for comment : see nodeca/js-yaml#196. "That's out of this library scope."

pascalgrimaud · 2017-12-10T18:01:02Z

For now, I think we should not merge this into generator-jhipster as it's specific for modules and globally for Yaml.
So let keep these functions in generator-jhipster-spring-cloud-stream

Depending if a lot of module need it or not, we can reconsider later:

merge request to js-yaml project
fork the project and maintain it
make a new project

So I'm closing this, I hope you agree, @antarus

antarus · 2017-12-11T13:03:13Z

I agree that in the state this PR is too oriented yaml, but I remain convinced that it is necessary to facilitate the integration of modules in the main project.

deepu105 · 2017-12-11T14:43:20Z

I'm gonna follow YAGNIY principle here, so if another module ever needs it we can reconsider Thanks & Regards, Deepu

…

On Mon, Dec 11, 2017 at 2:03 PM, Cédric ***@***.***> wrote: I agree that in the state this PR is too oriented yaml, but I remain convinced that it is necessary to facilitate the integration of modules in the main project. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6769 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDlF4F41h1kspt_SGqm03PwxHbB9_0tks5s_SgZgaJpZM4QvZ5f> .

jdk8 added 2 commits November 29, 2017 19:12

add function for add property in a yaml file

ff3d7c4

code from hipster-labs/generator-jhipster-spring-cloud-stream

correct unit test for windows

e98ae73

correct unit test on windows correct unit test for windows user

deepu105 requested changes Nov 29, 2017

View reviewed changes

deepu105 added pr: on-hold status: needs-discussion labels Nov 29, 2017

pascalgrimaud closed this Dec 10, 2017

jdubois added this to the 4.13.0 milestone Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods for work with yaml file #6769

Add methods for work with yaml file #6769

antarus commented Nov 29, 2017 •

edited

Loading

deepu105 left a comment

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 Nov 29, 2017

deepu105 commented Nov 29, 2017

antarus commented Nov 30, 2017

sendilkumarn commented Nov 30, 2017

deepu105 commented Nov 30, 2017

pascalgrimaud commented Nov 30, 2017

deepu105 commented Nov 30, 2017

PierreBesson commented Nov 30, 2017

deepu105 commented Nov 30, 2017

deepu105 commented Nov 30, 2017

PierreBesson commented Nov 30, 2017

pascalgrimaud commented Dec 1, 2017

deepu105 commented Dec 1, 2017

pascalgrimaud commented Dec 1, 2017

antarus commented Dec 1, 2017

deepu105 commented Dec 1, 2017

deepu105 commented Dec 1, 2017

antarus commented Dec 1, 2017 •

edited

Loading

pascalgrimaud commented Dec 10, 2017

antarus commented Dec 11, 2017

deepu105 commented Dec 11, 2017 via email

Add methods for work with yaml file #6769

Add methods for work with yaml file #6769

Conversation

antarus commented Nov 29, 2017 • edited Loading

deepu105 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepu105 commented Nov 29, 2017

antarus commented Nov 30, 2017

sendilkumarn commented Nov 30, 2017

deepu105 commented Nov 30, 2017

pascalgrimaud commented Nov 30, 2017

deepu105 commented Nov 30, 2017

PierreBesson commented Nov 30, 2017

deepu105 commented Nov 30, 2017

deepu105 commented Nov 30, 2017

PierreBesson commented Nov 30, 2017

pascalgrimaud commented Dec 1, 2017

deepu105 commented Dec 1, 2017

pascalgrimaud commented Dec 1, 2017

antarus commented Dec 1, 2017

deepu105 commented Dec 1, 2017

deepu105 commented Dec 1, 2017

antarus commented Dec 1, 2017 • edited Loading

pascalgrimaud commented Dec 10, 2017

antarus commented Dec 11, 2017

deepu105 commented Dec 11, 2017 via email

antarus commented Nov 29, 2017 •

edited

Loading

antarus commented Dec 1, 2017 •

edited

Loading