Skip to content

feat: allow configurable default value for boolean template variables (#84) - #145

Closed
Aaravanand00 wants to merge 3 commits into
joplin:masterfrom
Aaravanand00:add-boolean-default-value-support
Closed

feat: allow configurable default value for boolean template variables (#84)#145
Aaravanand00 wants to merge 3 commits into
joplin:masterfrom
Aaravanand00:add-boolean-default-value-support

Conversation

@Aaravanand00

@Aaravanand00 Aaravanand00 commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

AI Assistance Disclosure

AI was used to explore refactoring approaches and validate edge cases around boolean default handling. The final implementation was manually reviewed, simplified, and adjusted based on maintainer feedback. I have verified the behavior through the test suite and fully understand the changes introduced.

Summary

Adds support for specifying a configurable default value for boolean template variables.

Example:

enable_feature:
type: boolean
default: no

The default value is interpreted case-insensitively.
If no default is provided, existing behavior is preserved.
Invalid values fall back to true, matching previous behavior.

@nishantwrp nishantwrp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe if we're supporting default values, it should be done in a generic manner. You should probably extend src/variables/types/base.ts to support a default value.

And probably add some functions to validate the provided default.

@Aaravanand00

Aaravanand00 commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi @nishantwrp,
Thanks for the suggestion .

I’ve refactored the implementation so that the default field is now extracted generically in base.ts, and the boolean-specific interpretation is handled inside BooleanCustomVariable.

In short:

  • Added a protected extractDefault() method in base.ts to retrieve the raw default value.
  • Moved the boolean parsing logic into a separate parseBooleanDefault() method in boolean.ts.
  • Kept the existing behavior unchanged when no default is provided.
  • Invalid values still fall back to true, matching the previous behavior.

All existing tests pass, and the new tests around boolean defaults are still green.

Please let me know if you’d like any further adjustments I'm happy to work on it.

@nishantwrp nishantwrp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks, took a pass.

also a meta point, ptal at guidelines for ai-assisted contributions if applicable

import { encode } from "html-entities";

export class InvalidDefinitionError extends Error {}
export class InvalidDefinitionError extends Error { }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you please remove these linting changes. makes it harder to review.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

still not resolved

Comment thread src/variables/types/base.ts Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

make default part of the constructor

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe name it "rawDefault" so that each type can validate it internally

Comment thread src/variables/types/base.ts
@Aaravanand00
Aaravanand00 force-pushed the add-boolean-default-value-support branch from 6ac7a68 to 6a5b68d Compare March 3, 2026 17:53
@Aaravanand00

Copy link
Copy Markdown
Contributor Author

Hi @nishantwrp,

I’ve addressed the requested changes:

  • Moved default extraction logic into base.ts so it’s handled generically at the factory level.
  • Kept boolean-specific parsing isolated inside BooleanCustomVariable.
  • Ensured no override of createFromDefinition in boolean.ts.
  • Cleaned up unrelated changes and kept the diff minimal.

I also updated the PR description to align with the AI-assisted contribution guidelines, including the required disclosure section.

All tests are passing locally.

One quick question — does Joplin have regular community meetings or an org Discord/Slack channel for contributors? If there’s a link available, I’d really appreciate it. I’d like to stay more involved and follow discussions more closely.

Comment thread src/variables/types/base.ts Outdated
Comment thread src/variables/types/base.ts Outdated
Comment thread src/variables/types/base.ts
Comment thread src/variables/types/boolean.ts Outdated
@nishantwrp

Copy link
Copy Markdown
Collaborator

@Aaravanand00 you can go through https://github.com/joplin/gsoc/?tab=readme-ov-file#google-summer-of-code-2025 for the communication channels to join

…lt typing

- Update CustomVariable constructor to accept rawDefault (string | boolean | null)
- Extract and validate default value inside BooleanCustomVariable
- Throw InvalidDefinitionError for invalid boolean defaults
- Preserve backward compatibility by defaulting rawDefault to null
- Ensure all existing tests pass without modifying other variable types
@Aaravanand00
Aaravanand00 force-pushed the add-boolean-default-value-support branch from 6a5b68d to 7563ff1 Compare March 3, 2026 19:58
@Aaravanand00

Copy link
Copy Markdown
Contributor Author

Hi @nishantwr

Thanks for the detailed feedback — I’ve addressed all the suggested changes.

changes:

  • Changed rawDefault type from unknown to string | boolean | null to make the intent explicit and avoid loose typing.

  • Made rawDefault default to null in the base constructor to avoid breaking other variable types that call super(name, label) with only two arguments.

  • Moved raw default extraction logic inside createFromDefinition in base.ts as suggested.

  • Updated BooleanCustomVariable to strictly parse and validate the default value.

  • Now it throws InvalidDefinitionError if the boolean default cannot be parsed (instead of silently accepting invalid values).

  • Ensured that when no default is provided, behavior remains backward compatible.

All existing tests pass locally without modifying other variable types.

@nishantwrp nishantwrp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks, i think we're very close. added a few comments.

also, can you please update the documentation.

import { encode } from "html-entities";

export class InvalidDefinitionError extends Error {}
export class InvalidDefinitionError extends Error { }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

still not resolved

Comment thread src/variables/types/base.ts Outdated
Comment thread src/variables/types/base.ts
Comment thread src/variables/types/boolean.ts Outdated
Comment thread src/variables/types/boolean.ts
Comment thread src/variables/types/boolean.ts Outdated
Comment thread src/variables/types/boolean.ts
@Aaravanand00

Copy link
Copy Markdown
Contributor Author

I've addressed the requested changes:

  • Updated rawDefault typing to include number since YAML can parse numeric values
  • Added explicit validation for unsupported default value types
  • Updated parseBooleanDefault to return null instead of undefined
  • Adjusted select logic so no option is preselected when default is null
  • Improved error messages for invalid defaults

All tests are passing locally (41 tests).

@nishantwrp nishantwrp closed this Mar 5, 2026
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.

2 participants