-
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
feat: current vs changed #151
base: main
Are you sure you want to change the base?
Conversation
tsconfig.lib.json
Outdated
{ | ||
"extends": "./tsconfig.base.json", | ||
"exclude": ["node_modules"], | ||
"includes": ["**/*.ts", "**/*.test.ts"] |
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.
If the definitions of lib and spec are the same, there is no need to separate them, right?
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.
woops that's a mistake. one is supposed to exclude tests the other include it.
src/detect.ts
Outdated
} | ||
|
||
export function getCurrentUnchecked(currentBody: string): string[] { | ||
const currentLines = currentBody.split('\n') |
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 am concerned that having currentLines split occur twice may slow down the speed when the text becomes long.
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.
yep same thoughts, I've got local changes where the input changes from previousBody: string
to previousLines: string[]
getDiff, | ||
getCurrentChecked, | ||
getCurrentUnchecked | ||
} from './detect' | ||
|
||
async function run(): Promise<void> { | ||
try { | ||
if (action === 'detect') { | ||
const previousBody = getPreviousBody() | ||
const currentBody = getCurrentBody() |
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 split at here.
src/main.ts
Outdated
core.setOutput('checked', JSON.stringify(checked)) | ||
core.setOutput('unchecked', JSON.stringify(unchecked)) | ||
core.setOutput('changed', JSON.stringify(changed)) |
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 dont want to make breaking change here.
Just make another action type for current.
also I would be happy if you update README.
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.
can you propose the output shape you'd like?
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.
The existing function returned a list of checks and unchecks for changes. I hope there is no change in that area.
I think the ideal thing would be to do a current-detect action and return a list of checked and unchecked items.
There are also library updates, so I will build in the main branch. Please revert changes in dist/*. |
938e128
to
565067f
Compare
const currentBody = getCurrentBody() | ||
const checked = getCurrentChecked(currentBody.split('\n')) | ||
const unchecked = getCurrentUnchecked(currentBody.split('\n')) |
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.
const currentBody = getCurrentBody() | |
const checked = getCurrentChecked(currentBody.split('\n')) | |
const unchecked = getCurrentUnchecked(currentBody.split('\n')) | |
const currentLines = getCurrentBody().split('\n') | |
const checked = getCurrentChecked(currentLines) | |
const unchecked = getCurrentUnchecked(currentLines) |
@@ -3,10 +3,11 @@ | |||
"target": "es6", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */ | |||
"module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ | |||
"outDir": "./lib", /* Redirect output structure to the directory. */ | |||
"rootDir": "./src", /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */ |
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.
Could you please explain the reason for this change?
I think this is the cause of the error below.
Error: Cannot find module '/home/runner/work/checkbox-action/checkbox-action/lib/main.js'. Please verify that the package.json has a valid "main" entry
fixes: #150
getDiff
into separate functions.as
outside of a type predicate or without type narrowing: buddah kills a kitten)