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

Allow function to programmatically return default values #6

Open
rugk opened this issue Jun 3, 2019 · 7 comments
Open

Allow function to programmatically return default values #6

rugk opened this issue Jun 3, 2019 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest Hacktoberfest 2020, https://hacktoberfest.digitalocean.com

Comments

@rugk
Copy link
Member

rugk commented Jun 3, 2019

Possibly required for rugk/offline-qr-code#201...

TODO:

  • allow dev to specify a function instead of the actual default value…
  • automatically execute that function, if the value is accessed
  • Edit: AFAIK, you also need to exclude the function from being tried to get frozen, because this cannot be done for a function? 🤔 (in the end, this then also requires the unit tests to be adjusted, though I can do that, they are not well-documented yet)
@rugk rugk added enhancement New feature or request good first issue Good for newcomers labels Jun 3, 2019
@rugk rugk changed the title Allow function to programmatically return default Allow function to programmatically return default values Jun 3, 2019
@davidfloyd91
Copy link

It could very well be that I'm missing something, but it appears that you can already specify a function. I haven't run into obvious issues with freezing, perhaps because functions are just JS objects.

For example (as a partial fix for rugk/offline-qr-code#201):

davidfloyd91/offline-qr-code@6b63a5f

There are still issues with the above, but setting a function as a value in the defaultSettings object appears to work fine...?

@davidfloyd91
Copy link

And now I'm getting these warnings. Figured it couldn't be that simple:

Offline QR code [WARN] The following defined default value of type object is not frozen. It is recommend that all default options are frozen. Object { debugMode: false, popupIconColored: false, qrCodeType: "svg", qrColor: "#0c0c0d", qrBackgroundColor: "#4a4a4f", qrErrorCorrection: "Q", autoGetSelectedText: false, monospaceFont: false, qrCodeSize: {…}, qrQuietZone: 1, … }

@rugk
Copy link
Member Author

rugk commented Jun 4, 2019

Okay, anyway, that surprises me…

Or actually, not. Ah, obviously, you just run the function and use the return value… fine. 😄
I actually rather thought it could only run this function once the default value actually get's accessed. (in this case, this may not needed, but generally it could be computationally expensive or so, so one may only want to run it, if actually needed. Or, it may be good to get the "fresh" result, if that changes from time to time.)

Anyway, your change from davidfloyd91/offline-qr-code@6b63a5f did not made it into the PR branch, as it seems, so I cannot review it yet there.

@rugk
Copy link
Member Author

rugk commented Jun 4, 2019

Ah and that warning seems unrelated… it's actually about something else, possibly. (Did you check whether it appears before?) Also, it's just a warning…

@davidfloyd91
Copy link

Or actually, not. Ah, obviously, you just run the function and use the return value… fine. 😄
I actually rather thought it could only run this function once the default value actually get's accessed. (in this case, this may not needed, but generally it could be computationally expensive or so, so one may only want to run it, if actually needed. Or, it may be good to get the "fresh" result, if that changes from time to time.)

Okay I see, had a feeling I wasn't understanding you fully. If you think this solution is too expensive or otherwise not workable let me know. I'll play around with implementing something more like what you're describing.

Ah and that warning seems unrelated… it's actually about something else, possibly. (Did you check whether it appears before?) Also, it's just a warning…

I saw those warnings once or twice when I almost certainly didn't cause them (I hadn't touched defaultSettings at the time). Then I played around with defaultSettings, didn't see those warnings for a bit and figured all was well. Then later they were back. It makes more sense that they'd be unrelated, but I figured I'd mention them cause they sounded related.

Anyway, your change from davidfloyd91/offline-qr-code@6b63a5f did not made it into the PR branch, as it seems, so I cannot review it yet there.

I'll leave a comment at rugk/offline-qr-code#201

@rugk
Copy link
Member Author

rugk commented Jun 5, 2019

BTW, that feature for OfflineQR has been merged, but if you want, feel free to continue this issue here. You can take it as a refactoring task (i.e. behaviour now works in OfflineQR and should stay like this).

So this issue here is mostly about "optimizing" the function execution, i.e. making it only run once/only if needed…
(And as said, it may be of [more] use later for some other add-on's or so.)

@davidfloyd91
Copy link

Sounds good!

@rugk rugk added the hacktoberfest Hacktoberfest 2020, https://hacktoberfest.digitalocean.com label Sep 25, 2019
@rugk rugk removed the hacktoberfest Hacktoberfest 2020, https://hacktoberfest.digitalocean.com label Nov 7, 2019
@rugk rugk added the hacktoberfest Hacktoberfest 2020, https://hacktoberfest.digitalocean.com label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest Hacktoberfest 2020, https://hacktoberfest.digitalocean.com
Projects
None yet
Development

No branches or pull requests

2 participants