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

feat add project base config #128

Closed

Conversation

Luffy2004-c
Copy link

@Luffy2004-c Luffy2004-c commented Nov 24, 2024

Description

Related Issue

#126

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello @Luffy2004-c, thanks for opening your first pull request 😊! We really appreciate your work. Happy Coding 🎉🎊 !

gcop/__main__.py Outdated
@@ -73,14 +74,16 @@ def generate_commit_message(
str: git commit message with ai generated.
"""
gcop_config = get_config()

commit_template = (
ConfigFileHandleMixin().get_local_config().get("gcoprule", None)
Copy link
Owner

Choose a reason for hiding this comment

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

不太建议用 Mixin 这种写法,能不能直接写成一两个函数就好了?simple is good.

gcop/__main__.py Outdated
def init_project_command():
"""Initialize gcop config"""
logger.color_info("Initializing gcop config...")
command = InitConfigCommand()
Copy link
Owner

Choose a reason for hiding this comment

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

就几行代码,直接写会不会更方直观?

Copy link
Author

Choose a reason for hiding this comment

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

你是指把init_config的逻辑直接全部搬到这个函数里面来吗?

Copy link
Owner

Choose a reason for hiding this comment

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

我觉得逻辑可能可以化简一点 但也问题不大 你先写着

@Undertone0809
Copy link
Owner

缺文档,可以补充一下使用文档。

@Undertone0809
Copy link
Owner

最好创建个单测 mock 一下

@Luffy2004-c
Copy link
Author

GET!🙂

def _check_config_exist(self):
return self.config_file_path.exists()

def get_local_config(self) -> dict:
Copy link
Owner

Choose a reason for hiding this comment

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

不要用 dict,用 typing Dict

def _check_config_exist(self):
return self.config_file_path.exists()

def get_local_config(self) -> dict:
Copy link
Owner

Choose a reason for hiding this comment

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

我不知道具体里面有什么字段,可以用 dataclass 声明一下

return self.config_file_path.exists()


class InitConfigCommand(ConfigFileHandleMixin):
Copy link
Owner

@Undertone0809 Undertone0809 Nov 28, 2024

Choose a reason for hiding this comment

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

我想说我不太喜欢你这种写法,只有一个函数单独定义个类有什么意义吗,让 AI 给你优化一下

Copy link
Owner

@Undertone0809 Undertone0809 Nov 28, 2024

Choose a reason for hiding this comment

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

看看 fastapi,django 的源码看看他们写 Mixin 的动机是什么,不要乱套设计模式。



@dataclass
class LocalConfig:
Copy link
Owner

Choose a reason for hiding this comment

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

叫 ProjectConfig 是不是更合适?

Copy link
Owner

Choose a reason for hiding this comment

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

~/user/.zeeland/gcop 下的配置也能叫 LocalConfig


from gcop.utils import Color, logger, read_yaml

INIT_CONFIG_COMMAND = "init_project"
Copy link
Owner

@Undertone0809 Undertone0809 Nov 28, 2024

Choose a reason for hiding this comment

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

你没 get 到我的意思,我是说 gcop init_project 这个命令很奇怪,而且在命令行中我们一般用 gcop init_project

这里单独用一个常量, 其他 command 都没用常量,你不觉得你这种写法很奇怪吗?

@Undertone0809
Copy link
Owner

建议把 init_config.py 这个文件删除了

  • 首先这个 init_config.py 的名字含义不明,local config? project config? 我已经有一个 config,py 文件,这个文件是用来干嘛的?
  • 都是 config 为什么不选择都放在 config.py 里面呢?

@Undertone0809
Copy link
Owner

文档 也很重要,没有文档等于你没更新这个功能,向大家介绍要如何使用最新发布的功能。

gcop/__main__.py Outdated
"""Initialize gcop config"""
logger.color_info("Initializing gcop config...")
command = InitConfigCommand()
if command.handle():
Copy link
Owner

@Undertone0809 Undertone0809 Nov 28, 2024

Choose a reason for hiding this comment

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

@app.command(name="init-project")
@check_version_before_command
def init_project_config():
    """Initialize gcop project config"""
    logger.color_info("Initializing gcop project config...")

    cur_path = Path.cwd()
    config_path = cur_path / ".gcop" / "config.yaml"

    if config_path.exists():
        logger.color_info("Gcop config already exists.", color=Color.YELLOW)
        return

    try:
        config_path.parent.mkdir(parents=True, exist_ok=True)
        config_path.touch()
    except Exception as e:
        logger.color_info(f"Failed to create gcop config: {e}", color=Color.RED)
        return

直接用面向过程写一遍是不是更简单?

gcop/__main__.py Outdated
@@ -73,14 +78,13 @@ def generate_commit_message(
str: git commit message with ai generated.
"""
gcop_config = get_config()

commit_template = get_local_config().gcoprule or gcop_config.commit_template
Copy link
Owner

Choose a reason for hiding this comment

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

为什么这个字段不和 gcop_config 的 commit_template 字段保持一致?这样子会徒增理解成本。

@Undertone0809
Copy link
Owner

如果 project 都可以有自己的一套 config,为什么不选择和 gcop_config 使用完全一样的字段和配置呢?你当前的写法只能自定义 commit_template,如果你把 gcop_config 和 project_config 抽象成一套 pattern,是不是可以用相同的方式直接读取项目配置和全局配置呢?

@Luffy2004-c Luffy2004-c force-pushed the add-project-baseconfig branch from bc3b813 to bddbc6d Compare December 8, 2024 02:43
EXAMPLE_CONFIG = GcopConfig.get_example_config()


def check_model_config(new_model: dict) -> bool:
Copy link
Owner

Choose a reason for hiding this comment

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

使用 typing Dict,不要用 dict

if not hasattr(get_config, "_instance"):
get_config._instance = GcopConfig.from_yaml()

project_config_path = Path.cwd() / ".gcop" / "config.yaml"
Copy link
Owner

Choose a reason for hiding this comment

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

看起来感觉可以化简

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.

2 participants