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

[config]Support config license header comment style. #97

Merged
merged 12 commits into from
Apr 6, 2022

Conversation

dongzl
Copy link
Contributor

@dongzl dongzl commented Mar 19, 2022

Recently, I use this project to fix myself project license header, It is a GoLand project.

I find that the header comment style config at the languages.yaml and styles.yaml, and it doesn't support config.

The Go file license header comment style is //, like:

// Licensed to the Apache Software Foundation (ASF) under one
// ... ...

But I want the license header comment style is *, like:

/*
 * Licensed to the Apache Software Foundation (ASF) under one
 * ... ...
 */

So, I fix code and submit this PR.

I add the config languages at the .licenserc.yaml, and the license header comment style use this file config preferentially, if this file doesn't have this config, it use the default config at the languages.yaml.

@dongzl
Copy link
Contributor Author

dongzl commented Mar 19, 2022

Hi skywalking-eyes community, I don't understand why the project CI failure, who can help me, Thanks.

@wu-sheng wu-sheng requested a review from kezhenxu94 March 19, 2022 04:01
@@ -18,6 +18,7 @@
package config

import (
"github.com/apache/skywalking-eyes/pkg/comments"
Copy link
Member

Choose a reason for hiding this comment

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

@dongzl Please move this to line 25 and this should fix your lint, try run gofmt -s -w pkg/config/config.go to fix it automatically. Just a formatting issue compaint by the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dongzl Please move this to line 25 and this should fix your lint, try run gofmt -s -w pkg/config/config.go to fix it automatically. Just a formatting issue compaint by the CI.

@Superskyyy It's OK, Thanks very much.

@wu-sheng wu-sheng added this to the 0.3.0 milestone Mar 31, 2022
@wu-sheng
Copy link
Member

@kezhenxu94 Please take a look.

Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

IT's better to merge the user-configured settings with the default ones (

var commentStyles = make(map[string]CommentStyle)
), so you don't need to change the fix logic.

Deps deps.ConfigDeps `yaml:"dependency"`
Header header.ConfigHeader `yaml:"header"`
Deps deps.ConfigDeps `yaml:"dependency"`
Languages map[string]comments.Language `yaml:"language"`
Copy link
Member

Choose a reason for hiding this comment

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

This config should be under Header

.licenserc.yaml Outdated
Comment on lines 89 to 100
type: programming
color: "#00ADD8"
aliases:
- golang
extensions:
- ".go"
tm_scope: source.go
ace_mode: golang
codemirror_mode: go
codemirror_mime_type: text/x-go
language_id: 132
comment_style_id: SlashAsterisk
Copy link
Member

Choose a reason for hiding this comment

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

Please minimize the configs, we only need some of the fields, like comment_style_id and extensions,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please minimize the configs, we only need some of the fields, like comment_style_id and extensions,

Hi @kezhenxu94 , Thanks for reply, I will fix it.

@dongzl
Copy link
Contributor Author

dongzl commented Mar 31, 2022

Hi @kezhenxu94 , I considered about this way, but read code,
var commentStyles = make(map[string]CommentStyle) is private and comments/config.go's init method can't receive param, I don't know how to merge the config content, so I use this way now.

@kezhenxu94
Copy link
Member

Hi @kezhenxu94 , I considered about this way, but read code,
var commentStyles = make(map[string]CommentStyle) is private and comments/config.go's init method can't receive param, I don't know how to merge the config content, so I use this way now.

Hi @dongzl , the way I proposed needs some other changes for sure, you can

@dongzl
Copy link
Contributor Author

dongzl commented Mar 31, 2022

Hi @kezhenxu94 , I considered about this way, but read code,
var commentStyles = make(map[string]CommentStyle) is private and comments/config.go's init method can't receive param, I don't know how to merge the config content, so I use this way now.

Hi @dongzl , the way I proposed needs some other changes for sure, you can

Hi @kezhenxu94 , OK I understand, I will try it.

@dongzl dongzl requested review from kezhenxu94 and Superskyyy April 4, 2022 02:42
@@ -104,6 +106,8 @@ func (config *ConfigHeader) Finalize() error {
config.Paths = []string{"**"}
}

comments.OverrideLanguageCommentStyle(config.Languages)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@kezhenxu94 kezhenxu94 dismissed Superskyyy’s stale review April 6, 2022 01:54

Concerns are addressed

@kezhenxu94 kezhenxu94 merged commit 66a8bd6 into apache:main Apr 6, 2022
@kezhenxu94
Copy link
Member

@dongzl nice done! Can you open another pull request to add the usage into the documentation as well?

@dongzl
Copy link
Contributor Author

dongzl commented Apr 6, 2022

@dongzl nice done! Can you open another pull request to add the usage into the documentation as well?

Hi @kezhenxu94 , Thanks for your code review, I will finish documentation.

@dongzl dongzl deleted the support-config-language branch April 6, 2022 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants