Closed cheton closed 2 months ago
Open the branch in Web Editor • VS Code • Insiders
Open Preview
Latest commit: c38fafab2934d086401a352050e0af669a514e2c
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
🧪 No relevant tests |
🔒 No security concerns identified |
⚡ Key issues to review **Possible Bug:** The `validate` function in `src/index.js` throws an error if the `repo` option is not provided, but it does not check for other required fields or potential null values in other parts of the code. This could lead to runtime errors if other expected properties are missing. **Code Duplication:** There is some repetitive code in `src/index.js` for fetching and handling GitHub information. Consider refactoring to reduce duplication and improve maintainability. **Error Handling:** The async functions in `src/index.js` do not have try-catch blocks for handling exceptions from external API calls, which might lead to unhandled promise rejections. |
This pull request is automatically built and testable in CodeSandbox.
To see build info of the built libraries, click here or the icon next to each commit SHA.
Category | Suggestion | Score |
Maintainability |
✅ Extract the anonymous function inside getReleaseLine into a named function for better readability and maintainability___Suggestion Impact:The anonymous function inside getReleaseLine was extracted into a named function called fetchLinks, improving readability and maintainability. code diff: ```diff - const links = await (async () => { + const fetchLinks = async () => { if (prFromSummary > 0) { const pullRequestInfo = await getInfoFromPullRequest({ repo: repo, @@ -100,7 +103,9 @@ pull: null, user: null, }; - })(); + }; + + const links = await fetchLinks(); ```inside the getReleaseLine function into a named function. This will make the code more modular and easier to test.** [packages/changelog-github/src/index.js [72-103]](https://github.com/trendmicro-frontend/tonic-ui/pull/888/files#diff-afb6e24249ad20dcbc1dc8cc6af33d94f792347fb8dcdc242d5b9c82f43f3564R72-R103) ```diff -const links = await (async () => { +const fetchLinks = async (repo, prFromSummary, commitFromSummary, changeset) => { if (prFromSummary > 0) { const pullRequestInfo = await getInfoFromPullRequest({ repo: repo, pull: prFromSummary, }); const links = pullRequestInfo.links; if (commitFromSummary) { const shortCommitId = commitFromSummary.slice(0, 7); return { ...links, commit: `[\`${shortCommitId}\`](https://github.com/${repo}/commit/${commitFromSummary})`, }; } return links; } const commitToFetchFrom = commitFromSummary || changeset.commit; if (commitToFetchFrom) { const info = await getInfo({ repo: repo, commit: commitToFetchFrom, }); return info.links; } return { commit: null, pull: null, user: null, }; -})(); +}; +const links = await fetchLinks(repo, prFromSummary, commitFromSummary, changeset); + ``` Suggestion importance[1-10]: 7Why: Extracting the anonymous function into a named function enhances readability and maintainability, making the code easier to understand and test. | 7 |
Performance |
Use
___
**To improve performance, consider using | 7 |
Possible issue |
✅ Add a null check for options before accessing its properties in the validate function___Suggestion Impact:The commit added a null check for the options object before accessing its properties in the validate function, which aligns with the suggestion to prevent potential issues with undefined or null values. code diff: ```diff - if (!options?.repo) { + const repo = options?.repo; + if (!repo) { ```options before accessing its properties in the validate function.**
[packages/changelog-github/src/index.js [7]](https://github.com/trendmicro-frontend/tonic-ui/pull/888/files#diff-afb6e24249ad20dcbc1dc8cc6af33d94f792347fb8dcdc242d5b9c82f43f3564R7-R7)
```diff
-if (!options?.repo) {
+if (!options || !options.repo) {
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 6Why: Adding a null check before accessing properties helps prevent runtime errors due to undefined or null values, improving code robustness. | 6 |
Enhancement |
Use template literals for the error message to improve readability___ **To improve readability, consider using template literals for the error message in thevalidate function.**
[packages/changelog-github/src/index.js [8-10]](https://github.com/trendmicro-frontend/tonic-ui/pull/888/files#diff-afb6e24249ad20dcbc1dc8cc6af33d94f792347fb8dcdc242d5b9c82f43f3564R8-R10)
```diff
-throw new Error(
- 'Please provide a repo to this changelog generator like this:\n"changelog": ["@tonic-ui/changelog-github", { "repo": "org/repo" }]'
-);
+throw new Error(`Please provide a repo to this changelog generator like this:
+"changelog": ["@tonic-ui/changelog-github", { "repo": "org/repo" }]`);
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 5Why: Using template literals for error messages enhances readability and maintainability, but it's a relatively minor improvement compared to other potential code enhancements. | 5 |
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 74.78%. Comparing base (
8ab8835
) to head (c38fafa
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
/ask Write Jest tests for the changelog-github
package that mocks the getInfo
and getInfoFromPullRequest
functions from the @changesets/get-github-info
package. This test should include beforeEach
and afterAll
to clear and restore mocks.
Write Jest tests for the changelog-github
package that mocks the getInfo
and getInfoFromPullRequest
functions from the @changesets/get-github-info
package. This test should include beforeEach
and afterAll
to clear and restore mocks.
To write Jest tests for the changelog-github
package that mocks the getInfo
and getInfoFromPullRequest
functions from the @changesets/get-github-info
package, you can follow the example below. This test will include beforeEach
and afterAll
to clear and restore mocks.
First, ensure you have Jest installed as a dev dependency in your project. Then, create a test file, for example, src/index.test.js
, and add the following code:
import changelogFunctions from './index';
import { getInfo, getInfoFromPullRequest } from '@changesets/get-github-info';
jest.mock('@changesets/get-github-info');
describe('changelog-github', () => {
beforeEach(() => {
jest.clearAllMocks();
});
afterAll(() => {
jest.restoreAllMocks();
});
describe('getDependencyReleaseLine', () => {
it('should return an empty string if no dependencies are updated', async () => {
const changesets = [];
const dependenciesUpdated = [];
const options = { repo: 'org/repo' };
const result = await changelogFunctions.getDependencyReleaseLine(changesets, dependenciesUpdated, options);
expect(result).toBe('');
});
it('should return a formatted string with updated dependencies', async () => {
const changesets = [{ commit: 'commit1' }];
const dependenciesUpdated = [{ name: 'dep1', newVersion: '1.0.1' }];
const options = { repo: 'org/repo' };
getInfo.mockResolvedValueOnce({
links: { commit: 'https://github.com/org/repo/commit/commit1' }
});
const result = await changelogFunctions.getDependencyReleaseLine(changesets, dependenciesUpdated, options);
expect(result).toBe('- Updated dependencies [https://github.com/org/repo/commit/commit1]\n - dep1@1.0.1');
});
});
describe('getReleaseLine', () => {
it('should return a formatted string with PR and commit info', async () => {
const changeset = {
summary: 'This is a test summary\nPR: #123\nCommit: abcdefg\nAuthor: user1',
commit: 'commit1'
};
const options = { repo: 'org/repo' };
getInfoFromPullRequest.mockResolvedValueOnce({
links: { pull: 'https://github.com/org/repo/pull/123', user: 'https://github.com/user1' }
});
const result = await changelogFunctions.getReleaseLine(changeset, 'minor', options);
expect(result).toBe('\n\n- This is a test summary by [@user1](https://github.com/user1) in https://github.com/org/repo/pull/123');
});
it('should return a formatted string with commit info if no PR is provided', async () => {
const changeset = {
summary: 'This is a test summary\nCommit: abcdefg\nAuthor: user1',
commit: 'commit1'
};
const options = { repo: 'org/repo' };
getInfo.mockResolvedValueOnce({
links: { commit: 'https://github.com/org/repo/commit/abcdefg', user: 'https://github.com/user1' }
});
const result = await changelogFunctions.getReleaseLine(changeset, 'minor', options);
expect(result).toBe('\n\n- This is a test summary by [@user1](https://github.com/user1) in https://github.com/org/repo/commit/abcdefg');
});
});
});
jest.mock('@changesets/get-github-info')
statement mocks the getInfo
and getInfoFromPullRequest
functions from the @changesets/get-github-info
package.beforeEach
: Clears all mocks before each test to ensure no test affects another.afterAll
: Restores all mocks after all tests are done.getDependencyReleaseLine
: Tests the function with no dependencies updated and with dependencies updated.getReleaseLine
: Tests the function with PR and commit info and with only commit info.This setup ensures that the functions are tested in isolation with mocked dependencies, providing reliable and repeatable tests.
PR Type
enhancement, configuration changes, dependencies
Description
changelog-github
package to the workspace for changelog generation usingChangesets
.src/index.js
..changeset/config.json
to use@tonic-ui/changelog-github
.changelog-github
package.package.json
.Changes walkthrough 📝
8 files
.eslintrc.js
Add ESLint configuration for `changelog-github` package
packages/changelog-github/.eslintrc.js
changelog-github
package.babel.config.js
Add Babel configuration for `changelog-github` package
packages/changelog-github/babel.config.js
changelog-github
package.jest-setup.js
Add Jest setup for `changelog-github` package
packages/changelog-github/jest-setup.js
changelog-github
package.config.json
Update changelog configuration to use `@tonic-ui/changelog-github`
.changeset/config.json
@tonic-ui/changelog-github
.changesets-release.yml
Add build step for `changelog-github` in CI workflow
.github/workflows/changesets-release.yml
changelog-github
package in GitHub Actionsworkflow.
.eslintignore
Add `.eslintignore` for `changelog-github` package
packages/changelog-github/.eslintignore
.eslintignore
file to exclude build directories andnode_modules.
.npmignore
Add `.npmignore` for `changelog-github` package
packages/changelog-github/.npmignore - Added `.npmignore` file to exclude lock files.
rollup.config.mjs
Add Rollup configuration for `changelog-github` package
packages/changelog-github/rollup.config.mjs
changelog-github
package.1 files
index.js
Implement changelog generation functions
packages/changelog-github/src/index.js
lines.
2 files
package.json
Update dependencies to use `@tonic-ui/changelog-github`
package.json
@changesets/changelog-github
with@tonic-ui/changelog-github
in devDependencies.
package.json
Add `package.json` for `changelog-github` package
packages/changelog-github/package.json
package.json
for thechangelog-github
package.1 files
LICENSE
Add MIT License for `changelog-github` package
packages/changelog-github/LICENSE - Added MIT License file for the `changelog-github` package.