Skip to content
This repository has been archived by the owner on Mar 21, 2021. It is now read-only.

Conversation

ntorionbearstudio
Copy link

@ntorionbearstudio ntorionbearstudio commented Jun 1, 2020

Please make sure the below checklist is followed for Pull Requests.

  • Checks 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

jhipster/generator-jhipster#11398

Copy link
Member

@MathieuAA MathieuAA left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, this week is hell for me...

@@ -119,6 +119,10 @@ function writeEntities(subFolder) {
for (let i = 0, entityNames = Object.keys(configuration.entities); i < entityNames.length; i++) {
const filePath = path.join(subFolder, toFilePath(entityNames[i]));
const entity = updateEntityToGenerateWithExistingOne(filePath, configuration.entities[entityNames[i]]);
if (FileUtils.doesFileExist(filePath)) {
const destPath = path.join(subFolder, toPreviousStateFilePath(entityNames[i]));
Copy link
Member

Choose a reason for hiding this comment

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

Please use the full word: destinationPath

@@ -20,7 +20,7 @@
const fs = require('fs');
const path = require('path');
const ApplicationTypes = require('../core/jhipster/application_types');
const { toFilePath, readJSONFile } = require('../readers/json_file_reader');
const { toFilePath, toPreviousStateFilePath, readJSONFile } = require('../readers/json_file_reader');
Copy link
Member

Choose a reason for hiding this comment

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

I don't get what the function does just by looking at the name.
What does it do?

Copy link
Member

Choose a reason for hiding this comment

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

I've suggested another name, perhaps it's more suggestive

@@ -0,0 +1,5393 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file. We use NPM

* @param entityName the entity's name.
* @returns {string} the file's path.
*/
function toPreviousStateFilePath(entityName) {
Copy link
Member

Choose a reason for hiding this comment

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

getFilePathForPreviousStateOfEntity would be a better name

Don't hesitate to be verbose!


/**
* From an entity's name, gives the expected previous state file path.
* @param entityName the entity's name.
Copy link
Member

Choose a reason for hiding this comment

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

{String} missing

/**
* From an entity's name, gives the expected previous state file path.
* @param entityName the entity's name.
* @returns {string} the file's path.
Copy link
Member

Choose a reason for hiding this comment

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

{String}

Comment on lines +122 to +125
if (FileUtils.doesFileExist(filePath)) {
const destPath = path.join(subFolder, toPreviousStateFilePath(entityNames[i]));
fs.copyFileSync(filePath, destPath);
}
Copy link
Member

Choose a reason for hiding this comment

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

What does this block do?

@MathieuAA
Copy link
Member

Tests are missing BTW

@mshima
Copy link
Member

mshima commented Jun 4, 2020

@MathieuAA what about returning the last entity state instead of creating a new file?
We can pass the structure to the generators and the generator/blueprint save there instead, if required.

I don’t like the idea of creating -previous file by default

@MathieuAA
Copy link
Member

@mshima I like your idea. @ntorionbearstudio is @mshima idea compatible with your feature?

@MathieuAA
Copy link
Member

@ntorionbearstudio Hello there! This repo's source code is being moved to the generator, and this PR's additions too.
The next steps for me are:

I'll help you finish this PR if need be

@ntorionbearstudio
Copy link
Author

@mshima I like your idea. @ntorionbearstudio is @mshima idea compatible with your feature?

Yes ! I'm working on it this week :)

@pascalgrimaud
Copy link
Member

Thanks @ntorionbearstudio but be carefull. This repo will be archived, because jhipster-core has been merged into generator-jhipster

@ntorionbearstudio
Copy link
Author

Ok I'm working on updated generator-jhipster repo

@ntorionbearstudio
Copy link
Author

I moved what I needed on the generator-jhipster repo : jhipster/generator-jhipster#11419

Now the last entities states are passed to entity generator in param and no more -previous files are created as @mshima suggested.

So I'm closing this PR and continue to work on jhipster/generator-jhipster#11419

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

Successfully merging this pull request may close these issues.

4 participants