-
Notifications
You must be signed in to change notification settings - Fork 1
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 input url option for cli #203
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
import fs from 'node:fs' | ||
import { afterEach, describe, expect, it, vi } from 'vitest' | ||
import { getInputContent } from './getInputContent.js' | ||
|
||
vi.mock('node:fs') | ||
vi.mock('node:https') | ||
|
||
describe('getInputContent', () => { | ||
afterEach(() => { | ||
vi.restoreAllMocks() | ||
}) | ||
|
||
it('should read local file content when given a valid file path', async () => { | ||
const mockFilePath = '/path/to/local/file.txt' | ||
const mockFileContent = 'Local file content' | ||
|
||
vi.spyOn(fs, 'existsSync').mockReturnValue(true) | ||
vi.spyOn(fs, 'readFileSync').mockReturnValue(mockFileContent) | ||
|
||
const content = await getInputContent(mockFilePath) | ||
|
||
expect(content).toBe(mockFileContent) | ||
}) | ||
|
||
it('should throw an error if the local file path is invalid', async () => { | ||
const mockFilePath = '/invalid/path/to/file.txt' | ||
|
||
vi.spyOn(fs, 'existsSync').mockReturnValue(false) | ||
|
||
await expect(getInputContent(mockFilePath)).rejects.toThrow( | ||
'Invalid input path. Please provide a valid file.', | ||
) | ||
}) | ||
|
||
it('should download raw content from GitHub when given a GitHub blob URL', async () => { | ||
const mockGitHubUrl = 'https://github.com/user/repo/blob/main/file.txt' | ||
const mockRawUrl = | ||
'https://raw.githubusercontent.com/user/repo/main/file.txt' | ||
const mockGitHubContent = 'GitHub raw file content' | ||
|
||
const mockFetch = vi | ||
.spyOn(global, 'fetch') | ||
.mockImplementation( | ||
async () => new Response(mockGitHubContent, { status: 200 }), | ||
) | ||
|
||
const content = await getInputContent(mockGitHubUrl) | ||
|
||
expect(content).toBe(mockGitHubContent) | ||
expect(mockFetch).toHaveBeenCalledWith(mockRawUrl) | ||
}) | ||
|
||
it('should download content from a regular URL', async () => { | ||
const mockUrl = 'https://example.com/file.txt' | ||
const mockUrlContent = 'Regular URL file content' | ||
|
||
const mockFetch = vi | ||
.spyOn(global, 'fetch') | ||
.mockImplementation( | ||
async () => new Response(mockUrlContent, { status: 200 }), | ||
) | ||
|
||
const content = await getInputContent(mockUrl) | ||
|
||
expect(content).toBe(mockUrlContent) | ||
expect(mockFetch).toHaveBeenCalledWith(mockUrl) | ||
}) | ||
|
||
it('should throw an error when file download fails', async () => { | ||
const mockUrl = 'https://example.com/file.txt' | ||
|
||
vi.spyOn(global, 'fetch').mockImplementation( | ||
async () => new Response('', { status: 404, statusText: 'Not Found' }), | ||
) | ||
|
||
await expect(getInputContent(mockUrl)).rejects.toThrow( | ||
'Failed to download file: Not Found', | ||
) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import fs from 'node:fs' | ||
import { URL } from 'node:url' | ||
|
||
function isValidUrl(url: string): boolean { | ||
try { | ||
new URL(url) | ||
return true | ||
} catch { | ||
return false | ||
} | ||
} | ||
|
||
function isGitHubFileUrl(url: string): boolean { | ||
const parsedUrl = new URL(url) | ||
return parsedUrl.hostname === 'github.com' && url.includes('/blob/') | ||
} | ||
|
||
function readLocalFile(filePath: string): string { | ||
if (!fs.existsSync(filePath)) { | ||
throw new Error('Invalid input path. Please provide a valid file.') | ||
} | ||
return fs.readFileSync(filePath, 'utf8') | ||
} | ||
|
||
async function downloadGitHubRawContent(githubUrl: string): Promise<string> { | ||
const rawFileUrl = githubUrl | ||
.replace('github.com', 'raw.githubusercontent.com') | ||
.replace('/blob', '') | ||
return await downloadFile(rawFileUrl) | ||
} | ||
|
||
async function downloadFile(url: string): Promise<string> { | ||
const response = await fetch(url) | ||
if (!response.ok) { | ||
throw new Error(`Failed to download file: ${response.statusText}`) | ||
} | ||
const data = await response.text() | ||
return data | ||
} | ||
|
||
export async function getInputContent(inputPath: string): Promise<string> { | ||
if (!isValidUrl(inputPath)) { | ||
return readLocalFile(inputPath) | ||
} | ||
|
||
return isGitHubFileUrl(inputPath) | ||
? await downloadGitHubRawContent(inputPath) | ||
: await downloadFile(inputPath) | ||
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only GitHub gets special handling, I see. |
||
} | ||
Comment on lines
+13
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that the CLI is handling quite a lot under the Perhaps generating the raw URL could be left as the user's responsibility. I’d like to hear others’ opinions on this as well. Example:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For private repositories, GitHub's Therefore, this URL conversion feature would only support public repositories. But it is ok, I think now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, I was wondering if i should convert url. Yeah, only public repo is available for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO: I thought slightly extensive too, but it was a minor point of contention, so once merged, it looked good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I thought URL.canParse() could be used, but it looks like this was only supported in Node.js in v19.
We have not considered the minimum version of Node.js to be supported by the CLI, but since the LTS is v18, it would be better to support that.
So it looks good as it is 👍🏻
ref: https://developer.mozilla.org/en-US/docs/Web/API/URL/canParse_static