You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/08/20 08:27:22 UTC
[GitHub] [superset] sinhashubham95 opened a new pull request, #21143: fix(plugin-chart-handlebars): fixed import
sinhashubham95 opened a new pull request, #21143:
URL: https://github.com/apache/superset/pull/21143
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229957529
> I am facing the issue with both MacOS and Linux.
Ok, if you're running into trouble on MacOS then this needs additional attention. Can you check what versions of Node and npm you're using? My output below:
```
$ node -v && npm -v
v16.9.1
7.21.1
```
I understand the argument about this functionality being simple and I agree - however, if we want to incorporate this logic into Superset, I would strongly recommend adding full unit tests to ensure we have full test coverage for each supported function.
Ping @rusackas , any thoughts on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229953011
@villebro I am facing the issue with both MacOS and Linux. I understand this part that removing the dependency will avoid any fixes to be incorporated automatically in Superset, but this is what I believe around this, the helpers here are very straightforward, and adding any extra dependency for such functions won't add any benefit but just bloat up our core Superset. Using libraries like moment, etc. makes a lot of sense for the vast set of functionalities and the community support it gets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229960139
@villebro I am using `node v16.16.0` and `npm v8.11.0`. Also, I have already added test cases around the functions I added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas closed pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
URL: https://github.com/apache/superset/pull/21143
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1228079994
@kgabryje @lyndsiWilliams can you please review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229149532
@hughhhh @zhaoyongjie @stephenLYZ @michael-s-molina can you please check this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1228843902
@AAfghahi can you please check this pull request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229443528
> @villebro can you please check this?
@sinhashubham95 sure thing, will check in a few hours!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1404222109
@sinhashubham95 @villebro just checking in on this... wondering if I'm off base in my prior assessment.
The build issue was resolved quite some time ago, and I don't think the moment dependency is a big deal, personally.
Is anyone still clamoring to get this lib copied/pasted into our codebase, and willing to maintain it? If not, I think we should probably forego this change and close the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1230535889
@villebro are we good to merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1228785775
Storybook has completed and can be viewed at
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229965234
@villebro thanks a lot for these iterations, it's just a healthy discussion and learning for me. Sure, lemme know whenever we can merge this, it has kind of been open for long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1230998876
I _really, really_ appreciate all the work happening in this PR, but I'm a little nervous about this one... it seems like we're taking on much of the functionality of a whole library in terms of stability/security/maintenance. And not the whole library, including time and currency formatters, for example.
I don't think the optimization of removing moment is all that pertinent to the project, but the `exports not defined` one might be. I haven't noticed this error on Mac, and I don't have a Windows machine to test. Has there been much investigation into testing/resolving this error, other than the approach herein?
Last and maybe least (if I'm over-thinking this), but this seems to be a copy/paste of the just-handlebars-helpers codebase, not an original implementation and would be subject to the copyright of the original library. It's MIT licensed, so we would need to include the author's copyright, and I'm _not sure_ how including MIT licensed code directly within an Apache-licensed file works. ¯\\\_(ツ)_/¯
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229827706
@sinhashubham95 just so I understand this PR correctly, are we essentially copying over the functions from `just-handlebar-helpers`? While it may make it possible to drop some unnecessary requirements, we're already pulling in `moment` as it's needed elsewhere, so dropping `just-handlebars-helpers` won't remove that dependency. Also, I notice the lock file hasn't been updated, so we'd at least need to rebuild the main `package-lock.json` file which appears to still be referencing `just-handlebars-helpers`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229963319
> I am using `node v16.16.0` and `npm v8.11.0`. Also, I have already added test cases around the functions I added.
Oh my bad - the tests look great, thanks for adding those 👍 I'm leaning towards your proposed solution, thanks for all the iterations here. Let's also wait if anyone else has any feedback before proceeding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1228360138
Storybook has completed and can be viewed at
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229436978
# [Codecov](https://codecov.io/gh/apache/superset/pull/21143?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21143](https://codecov.io/gh/apache/superset/pull/21143?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d77a90f) into [master](https://codecov.io/gh/apache/superset/commit/d408393ba9bb6c81c661248521be93dd70c9e19b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d408393) will **decrease** coverage by `0.00%`.
> The diff coverage is `65.71%`.
```diff
@@ Coverage Diff @@
## master #21143 +/- ##
==========================================
- Coverage 66.40% 66.40% -0.01%
==========================================
Files 1783 1788 +5
Lines 68087 68191 +104
Branches 7261 7298 +37
==========================================
+ Hits 45215 45284 +69
- Misses 21007 21034 +27
- Partials 1865 1873 +8
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `52.33% <65.71%> (+0.04%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ars/src/components/Handlebars/HandlebarsViewer.tsx](https://codecov.io/gh/apache/superset/pull/21143/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvY29tcG9uZW50cy9IYW5kbGViYXJzL0hhbmRsZWJhcnNWaWV3ZXIudHN4) | `0.00% <0.00%> (ø)` | |
| [...lugins/plugin-chart-handlebars/src/helpers/html.ts](https://codecov.io/gh/apache/superset/pull/21143/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvaGVscGVycy9odG1sLnRz) | `33.33% <33.33%> (ø)` | |
| [...ugins/plugin-chart-handlebars/src/helpers/utils.ts](https://codecov.io/gh/apache/superset/pull/21143/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvaGVscGVycy91dGlscy50cw==) | `42.85% <42.85%> (ø)` | |
| [...lugin-chart-handlebars/src/helpers/conditionals.ts](https://codecov.io/gh/apache/superset/pull/21143/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvaGVscGVycy9jb25kaXRpb25hbHMudHM=) | `59.52% <59.52%> (ø)` | |
| [...ins/plugin-chart-handlebars/src/helpers/strings.ts](https://codecov.io/gh/apache/superset/pull/21143/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvaGVscGVycy9zdHJpbmdzLnRz) | `87.87% <87.87%> (ø)` | |
| [...lugins/plugin-chart-handlebars/src/helpers/math.ts](https://codecov.io/gh/apache/superset/pull/21143/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvaGVscGVycy9tYXRoLnRz) | `100.00% <100.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229429099
@villebro can you please check this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1228782155
@zhaoyongjie @stephenLYZ @michael-s-molina can you please check this pr? It would be very helpful, it has been open since long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229949438
@sinhashubham95 thanks for the clarification. Did I understand correctly that you're getting the export error when running on Windows? While I would like to help support a more diverse se of OSes, currently Superset does not officially support running the application on Windows. So if this is the main reason for removing `just-handlebars-helpers` then we need to consider the following tradeoffs:
- removing dependency on `just-handlebars-helpers` reduces dependencies, but at the same time we loose any fixes from the upstream package by copying over the current subset of functions
- Trying to support Windows may introduce other changes that can be a maintenance burden on the project. AFAIK none of the core committers are running Superset on anything but MacOS and Linux, so committing to support Windows in the absence of any dedicated core developers will likely be difficult. Again, it's great if we can support as wide an array of OSes as possible, but any additionally supported platform, be it implicit or explicit, will always introduce additional maintenance burden.
Out of curiosity, would running on a Linux server be an option? This is currently the recommended setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sinhashubham95 commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by GitBox <gi...@apache.org>.
sinhashubham95 commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1229939051
@villebro I have updated the package lock file.
The idea behind updating the package lock file is-
1. Just handlebars helpers usage was causing an issue saying `exports not defined` which is making the current handlebars plugin unusable in Superset.
2. Also, the plugin handlebars can be lighter by removing this dependency and adding the methods required directly. Here we have added a subset of the methods which is available in the helpers package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #21143: fix(plugin-chart-handlebars): Fixed issue with Helpers
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #21143:
URL: https://github.com/apache/superset/pull/21143#issuecomment-1500695098
Closing, but still happy to revisit this discussion if/when needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org