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

Support personalized templates #258

Merged
merged 9 commits into from
Nov 5, 2020

Conversation

folago
Copy link
Contributor

@folago folago commented Oct 19, 2020

I think this is the bare minimum for a PR, If you are willing to Merge I can improve it.
I am aware that this will "force" you to document the template, I am willing to add more docs if needed.

@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #258 into master will decrease coverage by 1.16%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
- Coverage   69.02%   67.85%   -1.17%     
==========================================
  Files          22       22              
  Lines        3693     3771      +78     
==========================================
+ Hits         2549     2559      +10     
- Misses        900      961      +61     
- Partials      244      251       +7     
Impacted Files Coverage Δ
config/config.go 50.30% <ø> (ø)
output/dot/dot.go 58.46% <57.14%> (-3.08%) ⬇️
output/md/md.go 73.52% <57.14%> (-1.41%) ⬇️
output/plantuml/plantuml.go 54.63% <57.14%> (-0.30%) ⬇️
datasource/datasource.go 22.22% <0.00%> (-12.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35be5d3...2f359ec. Read the comment docs.

@k1LoW k1LoW mentioned this pull request Oct 19, 2020
@folago folago force-pushed the gt-257-personalize-ER-template branch from d797f85 to c6a3e86 Compare November 3, 2020 17:40
@folago
Copy link
Contributor Author

folago commented Nov 3, 2020

Added the new functionality and updated the tests and the README.
I tried to follow your style but if something is not OK let me know and I'll fix it.

Copy link
Owner

@k1LoW k1LoW left a comment

Choose a reason for hiding this comment

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

@folago Thank you for your GREAT COMMIT!!!

I've made two comments, please check them out.

README.md Outdated
@@ -544,6 +544,9 @@ er:
# ER diagram (png/jpg) font (font name, font file, font path or keyword)
# Default is "" ( system default )
font: M+
# ER diagram dot format template, if specified this template file will be used
# in place of the defualt one to generate the dot file for graphviz
# template: schema.dot.tmpl
Copy link
Owner

Choose a reason for hiding this comment

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

Could you change to new syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot that commit during rebase, I'll remove it and keep the changes to the README in one place.

@@ -783,6 +786,26 @@ dict:
Table Definition: テーブル定義
```

### Personalized Templates
Copy link
Owner

Choose a reason for hiding this comment

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

👍

config/config.go Outdated
@@ -259,6 +261,8 @@ func (c *Config) LoadConfigFile(path string) error {
func (c *Config) LoadConfig(in []byte) error {
err := yaml.Unmarshal(in, c)
if err != nil {
log.Println(err)
log.Println("this should be cool", yaml.FormatError(err, true, true))
Copy link
Owner

Choose a reason for hiding this comment

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

Using yaml.FormatError is a very good idea. But could you exclude it from this Pull Request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that was a leftover from debug 😁, removing.

By the way, the last version of github.com/goccy/go-yaml has a more informative error message in case of invalid yaml.
Do you mind if I update it in go.mod?

@k1LoW k1LoW changed the title Issue 257 personalize er template Support personalized templates Nov 5, 2020
@k1LoW k1LoW added the enhancement New feature or request label Nov 5, 2020
@folago
Copy link
Contributor Author

folago commented Nov 5, 2020

@folago Thank you for your GREAT COMMIT!!!

Thanks 🙂

I've made two comments, please check them out.

I pushed some fixes to not lose track of what changed, when all is OK I'll rebase so it's cleaner.

Copy link
Owner

@k1LoW k1LoW left a comment

Choose a reason for hiding this comment

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

I found one more code that needs to be fixed, but nothing more.

THANK YOU!!! I think you can rebase now.

t.Error(err)
}
// use the templates in the testdata directory
c.Templates.PUML.Table = path.Join(testdataDir(), c.Templates.PUML.Table)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you would better use the filepath package.

Suggested change
c.Templates.PUML.Table = path.Join(testdataDir(), c.Templates.PUML.Table)
c.Templates.PUML.Table = filepath.Join(testdataDir(), c.Templates.PUML.Table)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do 🙂

When populated the templates specified in the config file will override
the default ones.
Updated README.md with a small paragraph on the personalized templates.
@folago folago force-pushed the gt-257-personalize-ER-template branch from 6e3493d to 2f359ec Compare November 5, 2020 13:49
@folago
Copy link
Contributor Author

folago commented Nov 5, 2020

Rebased 🙂

@k1LoW
Copy link
Owner

k1LoW commented Nov 5, 2020

YEAAAAAAAAAH!! 🎸 🎸 🎸 🎸 🎸 🎸 🎸

@k1LoW k1LoW merged commit fbd30a2 into k1LoW:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants