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

plots: special chars handling for vega plots paths #6900

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

pared
Copy link
Contributor

@pared pared commented Nov 1, 2021

Fixes: #6894

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Cannot test it for now. Marking as [WIP] until I can do that or get confirmation its working on windows.

@pared pared requested a review from a team as a code owner November 1, 2021 12:57
@pared pared changed the title plots: special chars handling for vega plots paths [WIP] plots: special chars handling for vega plots paths Nov 1, 2021
@shcheklein
Copy link
Member

Thanks @pared for addressing it quick! Is it WIP still?

Also tests failing and probably we should add a test for this bug, wdyt?

@pared
Copy link
Contributor Author

pared commented Nov 1, 2021

I don't have access to a Windows computer and won't have it for a few days, hence WIP. If you could confirm this fix works on windows too we could merge it faster. I will write a test.

@pared pared force-pushed the 6894_fix_id_assignment branch from 7421198 to 2a535c3 Compare November 3, 2021 23:03
@pared pared changed the title [WIP] plots: special chars handling for vega plots paths plots: special chars handling for vega plots paths Nov 3, 2021
@pared
Copy link
Contributor Author

pared commented Nov 3, 2021

@shcheklein I created a simple temporary test, to make sure all the special characters get converted. We can merge this change, however, this issue is not over. To make sure we do not get more of such issues, we should introduce testing for our generated HTML. For the beginning, we should try to parse the HTML, extract js and verify that err log is empty when executed. That requires some research, that's why I would opt for merging this fix and continuing proper HTML testing in the new issue.

@skshetry
Copy link
Member

skshetry commented Nov 4, 2021

For e2e tests, I'd suggest playwright for simple tests, it has nice integration with pytest. I have more experience on selenium, but playing with playwright, it looks really nice (this gist that I played with might be helpful: https://gist.github.com/skshetry/167b982372c522ad6f9e09973ecfc10b).

playwright supports all 3 platforms and 3 browsers, and has a guide for setting up CI in GHA.

I'd prefer we run this on a nightly basis on a separate repository though, as we need to depend on some browsers and these tests may be flaky.

@pared
Copy link
Contributor Author

pared commented Nov 4, 2021

I'd prefer we run this on a nightly basis on a separate repository though, as we need to depend on some browsers and > these tests may be flaky.

Another reason to evict rendering out of DVC.

@shcheklein
Copy link
Member

I've approved, @pared . Sorry never had intention to block this (I was satisfied with the answer- In a sense I trust that you and the team could figure out the best).

@pared
Copy link
Contributor Author

pared commented Nov 15, 2021

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Nov 15, 2021

rebase

βœ… Branch has been successfully rebased

@skshetry skshetry force-pushed the 6894_fix_id_assignment branch from 2a535c3 to 6742f6c Compare November 15, 2021 11:48
@pared pared force-pushed the 6894_fix_id_assignment branch from 6742f6c to bedc4ca Compare November 16, 2021 11:42
@efiop efiop merged commit e0fd079 into iterative:master Nov 16, 2021
@JulianWgs
Copy link

Thanks for the fix. When will this be released? :)

@pared
Copy link
Contributor Author

pared commented Nov 29, 2021

@JulianWgs It seems that draft release for 2.8.4 is already there. I hope that we can do that this week. I will check with other maintainers if there is anything blocking us/

@pared
Copy link
Contributor Author

pared commented Dec 6, 2021

@JulianWgs Regretfully we were not able to release yet, we have few major inner changes coming and the release needs to wait until we are sure everything works as intended. Meanwhile, if your use case cannot wait, I would recommend installing from master, if it's possible on your side.

@pared
Copy link
Contributor Author

pared commented Dec 8, 2021

@JulianWgs 2.9.0 containing this change has been released: https://github.com/iterative/dvc/releases/tag/2.9.0

@JulianWgs
Copy link

JulianWgs commented Dec 8, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: plots Related to the plots bugfix fixes bug
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

plots show: on Windows generates broken HTML
6 participants