Skip to content

Conversation

@sukjuhong
Copy link
Contributor

Summary

This pull request implements the add command for the bananass CLI.

Details

  • Generates a problem file for the given Baekjoon problem number.
  • Automatically creates the entry directory if it doesn't exist.
  • Skips generation if the problem file already exists.
  • Detects the module format (such as cjs, mjs, cts, mts) using the bananass.config file.
  • To prevent syntax errors, template files use the .txt extension.

How did you test this change?

I tested it using two files: add.test.js and parsers.test.js.

parsers.test.js uses a mock HTML file: 1000.html.

It verifies that the test case is correctly retrieved by using the mocked axios library and the mock HTML.

add.test.js checks whether the problem files are correctly generated based on the four extensions (cjs, mjs, cts, mts) in the fixtures directory.

Resolved Issues

No issues have been reported.


너무 심심해서 add 명령어를 구현해봤습니다.
최대한 루밀님의 코드를 따라해보려고 했는데 혹시 놓친 부분이 있을까 걱정이네요.
다음에는 이슈를 발행하고 진행하는게 좋겠죠? 말씀드리지 않고 진행한 것은 죄송합니다..
추가적으로 변경이 필요하거나 테스트가 추가되어야 한다면 말씀해주세요!

그리고 open 라이브러리 기여에 성공했습니다! 버전을 10.1.2로 올리시고 Q&A의 Q. WSL 환경에서 파이어폭스 등 설치되어 있지 않은 브라우저로 시도하거나, Error: Wslview is not supported as a default browser 같은 오류가 발생할 경우에는 어떻게 해야하나요? 부분은 삭제해도 좋을 것 같아요. 사실 오픈소스 기여를 이제 처음 시작하는데 루밀님 덕분에 쉽게 좋은 경험을 하게 된거 같아요. 정말 감사드립니다. 앞으로도 심심할때마다 찾아오겠습니다. 🤣

Copilot AI review requested due to automatic review settings May 2, 2025 09:10
@sukjuhong sukjuhong requested a review from lumirlumir as a code owner May 2, 2025 09:10
@vercel
Copy link

vercel bot commented May 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
npm-bananass-websites-eslint-config-bananass ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2025 9:56am
npm-bananass-websites-vitepress ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2025 9:56am

@github-actions github-actions bot added 🏷️ scope: bananass Auto-generated label based on Conventional Commits specification for GitHub release notes 🏷️ type: feat Auto-generated label based on Conventional Commits specification for GitHub release notes labels May 2, 2025
@github-actions
Copy link

github-actions bot commented May 2, 2025

Labels have been automatically applied based on the Conventional Commits specification.🏷️

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new "add" command for the bananass CLI that generates problem files for specified Baekjoon problem numbers. Key changes include:

  • Adding the "add" command to CLI command registrations and descriptions.
  • Introducing new solution templates for various module formats (cjs, mjs, cts, mts).
  • Including tests and parsers to validate file generation and case parsing.

Reviewed Changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/bananass/src/core/cli/descriptions.js Adds a new description for the "add" command.
packages/bananass/src/commands/index.js Imports and exports the new add command.
packages/bananass/src/commands/bananass-add/templates/* Introduces solution templates for different module formats.
packages/bananass/src/commands/bananass-add/parsers* Implements parser logic and tests for fetching problem test cases.
packages/bananass/src/commands/bananass-add/add.js Implements the core logic for file generation and configuration resolution.
packages/bananass/src/cli/bananass-add.js Configures the CLI command options and integrates the add command.
Files not reviewed (2)
  • packages/bananass/package.json: Language not supported
  • packages/bananass/src/commands/bananass-add/fixtures/.gitignore: Language not supported

@codecov
Copy link

codecov bot commented May 2, 2025

Bundle Report

Changes will decrease total bundle size by 9 bytes (-0.0%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
websites-vitepress-esm 863.55kB -9 bytes (-0.0%) ⬇️

Affected Assets, Files, and Routes:

view changes for bundle: websites-vitepress-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
@localSearchIndexroot.*.js 1 bytes 95.34kB 0.0%
@localSearchIndexen.*.js -10 bytes 8.84kB -0.11%

@codecov
Copy link

codecov bot commented May 2, 2025

Codecov Report

Attention: Patch coverage is 87.11864% with 38 lines in your changes missing coverage. Please review.

Project coverage is 96.67%. Comparing base (a8fd7dc) to head (961e5c3).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
packages/bananass/src/cli/bananass-add.js 48.78% 21 Missing ⚠️
packages/bananass/src/commands/bananass-add/add.js 92.77% 13 Missing ⚠️
...ages/bananass/src/commands/bananass-add/parsers.js 94.02% 4 Missing ⚠️
@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
- Coverage   96.95%   96.67%   -0.29%     
==========================================
  Files         157      160       +3     
  Lines       10055    10347     +292     
  Branches       16       16              
==========================================
+ Hits         9749    10003     +254     
- Misses        306      344      +38     
Files with missing lines Coverage Δ
...ckages/bananass/src/commands/bananass-add/index.js 100.00% <100.00%> (ø)
packages/bananass/src/commands/index.js 100.00% <100.00%> (ø)
packages/bananass/src/core/cli/descriptions.js 100.00% <100.00%> (ø)
...ages/bananass/src/commands/bananass-add/parsers.js 94.02% <94.02%> (ø)
packages/bananass/src/commands/bananass-add/add.js 92.77% <92.77%> (ø)
packages/bananass/src/cli/bananass-add.js 70.83% <48.78%> (-29.17%) ⬇️

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sukjuhong
Copy link
Contributor Author

I think an e2e test for this command is necessary to pass the coverage.

@lumirlumir
Copy link
Owner

@sukjuhong

안녕하세요, 우선 open 패키지 첫 기여 축하드립니다!

현재 PR 작성하시느라 수고하신 점 정말 감사히 생각하고 있습니다! 다만, 현재 PR은 아래와 같은 이유로 승인이 어렵습니다.

  1. 제가 구상하고 있는 add (create)의 방향성과 현재 PR의 방향성은 많이 다릅니다.

    현재 구현을 보면 백준 홈페이지에 HTTP 요청을 보내 HTML을 받아와서 cheerio를 통해 파싱한 후 해당 테스트 케이스를 받아오는 것으로 보여집니다.

    다만, 저는 홈페이지에 직접 HTTP 요청을 보내 테스트 케이스를 받아오는 방향으로는 구현을 하지 않을 예정입니다.

    바나나 프레임워크는 자유로운 라우팅 구조를 지향합니다. 예를 들어, 아래와 같이 테스트 케이스 변수를 문제 풀이 파일 안에 선언해서 활용한다 가정하겠습니다. (백준 1000번 문제를 가정하겠습니다.)

    const testcases = [
        {
            input: '1 2'
            output: '3'
        },
        // ...
    ]

    이 파일은 사용자가 원하는 어떤 위치에든 존재할 수 있습니다. 예를 들어, bananass/1000/testcases.js 파일에 따로 위치할 수도 있고, bananass/1000.js 파일 내부에 solution 함수 로직과 함께 위치할 수도 있으며, 루트 레벨에 testcases 폴더만 따로 만들어서 testcases/1000.js 파일과 같이 작성하여 관리할 수도 있습니다.

    테스트 케이스 변수가 담긴 파일의 위치는 사용자의 취향에 따라 어디든 위치할 수 있습니다. 이런 이유로, 테스트 케이스를 파싱해서 가지고 오게 되면, 이 파일을 어디 위치시키는 것이 올바른지에 대한 문제가 생깁니다.

    물론, '프레임워크'라는 특성 상 테스트 케이스의 위치에 제약을 두고 구조를 설계할 수도 있습니다. (boj-cli 패키지가 그러합니다.) 하지만, 제가 바나나 프레임워크를 만들면서 생각하는 철학인 '단순성, 자유로움'과는 거리가 멀어지고, 자바스크립트 사용자들이 생각하는 일반적인 모듈 운용 방식과는 괴리감이 생기게 됩니다. 그리고 테스트 케이스 위치에 제약을 걸어버리면, 프레임워크가 자바스크립트/타입스크립트만 지원하기 때문에 얻을 수 있는 이점이 상당히 감소합니다.

  2. 바나나 프레임워크 내부적으로 사용하는 모듈들에 대한 익숙함이 부족합니다.

    이 PR에 많은 시간을 투자하신 점은 정말 감사하게 생각하고 있습니다! 다만, 아직 바나나 프레임워크 내부 모듈들에 대한 익숙함이 부족하기 때문에 현재 제출해주신 코드는 현실적으로 많은 리팩토링이 필요합니다. (이건 실력 차원의 문제가 아니라, 특정 프로젝트에 대한 익숙함의 문제입니다.)

    사실, 수백줄이 넘어가는 코드 기여는 쉬운 일이 아닙니다. 저도 수백줄이 넘는 코드를 외부 프로젝트에 기여하기까지 정말 오랜 시간이 걸렸습니다.


거의 모든 오픈 소스에서 통용되는 얘기지만, 오픈 소스에 대형 기여를 하기 전에는 항상 이슈를 남겨서 팀 멤버들의 의견을 듣는 과정이 필요합니다.

단순 문서 수정, 버그 수정 정도는 직접 PR을 넣어도 큰 무리가 없습니다. 다만, 수십에서 수백줄이 넘어가는 기능 제안같은 경우, 해당 기능이 팀의 방향성과 일치해야 하며, 팀이 구상하는 큰 그림 안에 위치해야 하기 때문에, 앞선 논의 없이 PR을 보낼 경우 기대와는 다른 결과를 얻을 가능성이 매우 높습니다.

이러한 이유들로, 현재 PR은 우선 보류 예정이며, 추후 Close 될 수도 있다는 점 알려드립니다.


혹시 이런 큰 부분 말고 다른 작은 부분들에 기여 의사가 있으시다면, 깃허브 디스커션에 글을 남겨주세요. 제가 적당한 부분을 선별해서 할당해드리도록 하겠습니다!

@lumirlumir lumirlumir marked this pull request as draft May 2, 2025 14:27
@sukjuhong
Copy link
Contributor Author

@lumirlumir
앗 제가 섣불리 기능을 판단한 감이 있었네요..
이 기능은 뒤로 미뤄두고 다른 부분으로 고민해보겠습니다!

거의 모든 오픈 소스에서 통용되는 얘기지만, 오픈 소스에 대형 기여를 하기 전에는 항상 이슈를 남겨서 팀 멤버들의 의견을 듣는 과정이 필요합니다.

또 이런 부분 알려주셔서 감사합니다. 아직 오픈 소스 새내기라 배울 점이 많네요.
덕분에 다시금 개발에 재미가 붙습니다.

@lumirlumir lumirlumir added blocked This change can't be completed until another issue is resolved needs design Important details about this change need to be discussed labels May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked This change can't be completed until another issue is resolved 🏷️ scope: bananass Auto-generated label based on Conventional Commits specification for GitHub release notes 🏷️ type: feat Auto-generated label based on Conventional Commits specification for GitHub release notes needs design Important details about this change need to be discussed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants