Add push config

[Moret84]

This is the same PR as #74. The author unexpectedly closed it. I am interested in the feature, as others seem to be. I rebased the branch on dev, as requested in CONTRIBUTING.md. Please let me know if some extra work is needed before merging the PR.

@evandeininger, I kept your name. My intention is absolutely not to appropriate your code. Let me know if it bothers you.

I couldn't test well the feature, as there seems to be an issue with ts-node, preventing the npm run dev command to execute the soft. I am not used to developing in Node, so I haven't found how to solve it.

[github-actions]

Stale pull request message

[di-sukharev]

@Moret84 long time no see :)

to test this you should run npm run build and then npm run start -- config set <your_config>=true/false then running npm run start should run the local vevrsion, so you are able to test it locally

[Moret84]

Hello @di-sukharev,
Thanks for your help, I have been able to do some tests and fix some bugs ;).
However, I could not get my config key working. After some debugging, I have found something in the code that could explain the reason, at validator level, in config.ts:

 
 const config = iniParse(configFile);

  for (const configKey of Object.keys(config)) {
    if (
      !config[configKey] ||
      ['null', 'undefined'].includes(config[configKey])
    ) {
      config[configKey] = undefined;
      continue;
    }

config[configKey] is assigned to undefined if the ini value is false. Hence, any configuration key set to false will be ignored, won't be ?
Is this an expected behavior ? The existing OCO_DESCRIPTION key is concerned as well, IMO.
What do you think about this ?

Thanks for your read and your work

[di-sukharev]

@Moret84 hey, i dont remember having this issue with boolean configs, could you confirm that false is set as undefined for all boolean configs?

an addition: let's make the config false by default, not sure if people want to push instantly, let them configure it

one more question to the PR: are you sure that when the config is set to true it's bypassing the prompt do you want to run git push? i dont see any code that handles this, so now it looks like users are asked if they want to git push and if the config is set to false the push wont happen anyway :)

[Moret84]

@Moret84 hey, i dont remember having this issue with boolean configs, could you confirm that false is set as undefined for all boolean configs?

Yeah, this is what the !config[configKey] || part of the if does.

an addition: let's make the config false by default, not sure if people want to push instantly, let them configure it

one more question to the PR: are you sure that when the config is set to true it's bypassing the prompt do you want to run git push? i dont see any code that handles this, so now it looks like users are asked if they want to git push and if the config is set to false the push wont happen anyway :)

You are right.
In my opinion it would be good to keep the prompt when the user has not set the OCO_GITPUSH variable (i.e. the variable is undefined).
What do you think about this behavior ?

[Moret84]

Hi @di-sukharev,

I have removed the false to undefined conversion. I have also improved the push prompt behavior:

• if not defined the prompt appears
• if defined and true the prompt does not appears and push is performed
• if defined and false no prompt no push

From my point of view the PR is ready. Please consider review it as I miss this feature 🤪

Thanks for your work
Moret

[github-actions]

Stale pull request message

[di-sukharev]

@Moret84 could you please fix the conflict and lets merge it <3

[Moret84]

@di-sukharev done

[di-sukharev]

@Moret84 have you tested the build and run it?
do npm run build && npm run start on the opencommit repo when there are staged changes, you can do any change, test it and then revert back

[di-sukharev]

you can also do npm run start -- config set KEY=VALUE && npm run start

[Moret84]

@di-sukharev Yes I have tested setting the variable to true, false and not setting at all. Behavior seems OK to me.

[dangnhdev]

@di-sukharev Let's review and merge it 🚀🚀🚀

[di-sukharev]

will be in the next version

[di-sukharev]

hey guys, it broke the test, but it is expected as we dont wait for the do you want to push? now, but could you make the config false by default? this will solve the test https://github.com/di-sukharev/opencommit/actions/runs/8476583098/job/23226316086

[Moret84]

@di-sukharev I am afraid I haven't understood. By default if the variable is not set, the prompt appears so it should be expected by the test. I can't reproduce the failed result for the test https://github.com/di-sukharev/opencommit/actions/runs/8476583098/job/23226316086 on my side

[di-sukharev]

hmm.. i may be mistaken, thanks, will take a look into it