Skip to content
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

CLI Logic Function - Enable #700

Merged
merged 9 commits into from
Dec 13, 2023

Conversation

keeramis
Copy link
Contributor

Description

Implements enable command to enable a Logic Function

How to Test

particle lf enable --name myLF

Related Issues / Discussions

https://app.shortcut.com/particle/story/124133/implement-enable-command

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA
  • Problem and solution clearly stated
  • Tests have been provided
  • Docs have been updated
  • CI is passing

@keeramis keeramis changed the base branch from master to feature/logic-function December 13, 2023 16:03
@keeramis keeramis marked this pull request as ready for review December 13, 2023 16:04
@@ -334,19 +334,23 @@ module.exports = class LogicFunctionsCommand extends CLICommandBase {
console.log(org, filepath);
}

async disable({ org, name, id }) {
async disable({ org, name, id }, status) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather to extract the inner functions that disable / enable the logicFunction instead of giving a new responsibility to disable function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean with that is to create a new function enable and extract the logic to change the status.
Something important if the user execute twice the enable command the logic-function should remains enable. Same thing with disable command

Copy link
Contributor Author

@keeramis keeramis Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something important if the user execute twice the enable command the logic-function should remains enable.

It will still remain to be enabled. It's the same functionality other than the bit flip so I think we can keep it under the same function? The only other difference is the output logs which is different for enable and disable. The important point is that the functionality is the same with different bits for false/true for the logic_function.enabled so I would like to keep it under the same function unless I am missing something here?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also tested enabling in the console, disabling in the CLI and vice verse.
And enabling and disabling multiple times work as expected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the thing is the function name is disable and do something else than disable a logicFunction. That's the main reason.

Copy link
Contributor

@hugomontero hugomontero Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to keep the same functionality then change the function name to updateStatus or something that specifies the actual thing the function is doing.

Copy link
Contributor

@hugomontero hugomontero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@keeramis keeramis merged commit 806b240 into feature/logic-function Dec 13, 2023
5 checks passed
@monkbroc monkbroc deleted the feature/sc-123399/enable branch January 19, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants