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

[MIG][14.0] Base #4

Conversation

StefanRijnhart
Copy link

No description provided.

"merged_modules": {
"partner_bank_active": "base"
},
"__comment__": "The members below are only used to help the analysis process",
Copy link

Choose a reason for hiding this comment

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

Yikes! those __comment__... 🙈

Just use other format. 🤷‍♂️ See legalsylvain/server-tools#4 (review).

Copy link
Author

Choose a reason for hiding this comment

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

Fine by me.

@@ -0,0 +1,123 @@
---Models in module 'base'---
Copy link

Choose a reason for hiding this comment

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

Now that we're in the works of refactoring and all that stuff, I have to tell you something: I hate the syntax of these files.

Couldn't we just have a YAML, which supports proper comments and syntax highlighting, and besides allows automated parsing to check migration completion status? Or even a CSV... but not this please 🙈

(Note: of course just a personal preference and absolutely not important. But really, this is awful 😆)

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @yajo. Well it's a distinct point. You're probably right. But it requires a big extra work. Feel free to make a PR against upgrade_analysis module, that is responsible of the generation of the files.

@legalsylvain
Copy link
Owner

Thanks !

@legalsylvain legalsylvain mentioned this pull request Dec 7, 2020
@legalsylvain
Copy link
Owner

cherry picked here. #5

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.

3 participants