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

Fix plausible loading #909

Merged
merged 6 commits into from Sep 18, 2022
Merged

Fix plausible loading #909

merged 6 commits into from Sep 18, 2022

Conversation

StefanUlbrich
Copy link
Contributor

Fixes #905

However, tags are given to the add_js_file in the form of keyword arguments. The data-domain has a minus in it which disqualifies it as a python identifier, yet the fix works. I would have expected that python complains so I don't know if this is an acceptable solution. Comments?

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks great to me! Thanks for the fix. I believe that the **kwargs pattern is a nice way to allow for - in Python arguments so it seems fine

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

oops I spoke too soon! can you please get the tests passing? :-)

@StefanUlbrich
Copy link
Contributor Author

oops I spoke too soon! can you please get the tests passing? :-)

Well, OK. I adjusted the unit tests accordingly ;) .. They run locally and pre-commit inclusive

Having no-valid identifiers as keys in **kwargs does not seem that abnormal

@StefanUlbrich
Copy link
Contributor Author

I accidentally added package.lock. Is this a problem?

tests/test_build.py Outdated Show resolved Hide resolved
@choldgraf
Copy link
Collaborator

Yes please don't modify package-lock.json unless it is relevant to this PR. I tried deleting it in the files but it looks like we just need to revert it to whatever is on main.

It looks good to me assuming that tests are happy + package lock is reverted.

@StefanUlbrich
Copy link
Contributor Author

Yes please don't modify package-lock.json unless it is relevant to this PR. I tried deleting it in the files but it looks like we just need to revert it to whatever is on main.

It looks good to me assuming that tests are happy + package lock is reverted.

I think that the tests were happy (I cannot trigger them) and I somehow managed to get the file reverted (that took much longer than it should have). I rebased on main too along the process. @choldgraf , please let me know if there are any more issues

@StefanUlbrich
Copy link
Contributor Author

Gentle reminder :-)

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Looks good to me - thanks!

@choldgraf choldgraf merged commit 892620e into pydata:main Sep 18, 2022
@jarrodmillman jarrodmillman added this to the 0.11 milestone Sep 19, 2022
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.

Plausible script generation results in malformed HTML
3 participants