-
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
Windows repo maker #2
base: 1
Are you sure you want to change the base?
Conversation
…ewel-case into windows-repo-maker
README.md
Outdated
``` | ||
#### Source dir layout | ||
Layout example: | ||
* `src` |
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 think it is better to move this logic to config. This tool should be tied to specific layout of source data, I think.
src/index.ts
Outdated
import { getMessageOfError } from './utils.js'; | ||
import { initConfiguration } from './config.js'; | ||
|
||
async function main(): Promise<number> { |
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.
Looks like the better place for this logic is cli.ts
. index.ts
should be library API (and cli
could use public API from index
).
src/jewel-case.ts
Outdated
import { configuration } from './config.js'; | ||
import { WindowsRepoBuilder } from './windows-repo-builder.js'; | ||
|
||
export async function plan(outDir: string, sourceDir: string): Promise<void> { |
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.
And this module looks like index.ts
:)
README.md
Outdated
@@ -12,3 +12,84 @@ Lifecycle commands: | |||
| `npx pnpm lint` | lint all files | | |||
| `npx pnpm lint-staged` | lint all staged files | | |||
| `npx pnpm test` | run unit tests | | |||
|
|||
## Usage | |||
#### CLI commands: |
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.
Let's make a separate PR only with CLI support and basic library API (and maybe with config support). Without any repo builders and Artifactory logic. It should be much easier to review and merge.
@@ -1,8 +1,11 @@ | |||
{ | |||
"type": "module", |
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.
Do we really need this in "dev" package.json
?
@@ -0,0 +1,12 @@ | |||
{ |
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.
Do we need this tasks file at all?
README.md
Outdated
#### CLI commands: | ||
Build repositories according to config: | ||
```sh | ||
jewel-case plan <repo-out> [source-dir] |
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 think it is better to pass all the things through config. Because output directory is only applicable to static-file repos, but not for Stores (like Snapcraft).
src/config.ts
Outdated
|
||
async init(): Promise<void> { | ||
const resolvedPath = path.resolve(process.cwd(), 'jewel-case.config.mjs'); | ||
if (resolvedPath && fs.existsSync(resolvedPath)) { |
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.
Should we really check the existence of file? It is a lot of reasons why import could fail (for example, a syntax error).
…clude/exclude globs for tests
…figuration file support in `extends` by TypeScript 5.0)
What is done:
What's need: