Skip to content

e_mark_point() uses grep() to mark Serie is not good #80

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

Closed
shrektan opened this issue Jun 7, 2019 · 0 comments · Fixed by #81
Closed

e_mark_point() uses grep() to mark Serie is not good #80

shrektan opened this issue Jun 7, 2019 · 0 comments · Fixed by #81

Comments

@shrektan
Copy link
Contributor

shrektan commented Jun 7, 2019

Hi,

This is the best package to exploit Echarts. I really love it.

However, I've noticed an issue on e_mark_point() (also on e_mark_line() and e_mark_area()) that is the matching of the serie param uses .get_index(), which uses grep(), the regular expressions.

I don't understand the logic behind these three special functions. In my opinion, it's not consistent with other functions. More importantly, it makes the programming use of these functions difficult. Imagine if the series names contain character like () or the names like "abc" and "abcd".

For example:

library(echarts4r)
x <- data.frame(a = 1:10, b = 1:10)
e_chart(x, 'a') %>%
  e_line('b', name = 'aa(bb)', smooth = TRUE, symbol = 'none') %>%
  e_mark_point('aa(bb)', data = list(xAxis = 5, yAxis = 5, value = 5))
  # Won't be marked unless using `aa\\(bb\\)`

and

library(echarts4r)
x <- data.frame(a = 1:10, b = 1:10, c = 1:20)
e_chart(x, 'a') %>%
  e_line('b', name = 'abcdefg', smooth = TRUE, symbol = 'none') %>%
  e_line('c', name = 'abcd', smooth = TRUE, symbol = 'none') %>%
  e_mark_point('abcd', data = list(xAxis = 5, yAxis = 5, value = 5))
  # the two series are both marked (click on the legend of abcd you will understand what I mean)
  # but we only want the second. Have to use `^abcd$`

The related Code

index <- .get_index(e, serie)

echarts4r/R/utils.R

Lines 550 to 555 in 5224fba

.get_index <- function(e, serie){
serie <- paste0(serie, collapse = "|")
purrr::map(e$x$opts$series, "name") %>%
unlist() %>%
grep(serie, .)
}

My suggestion

My suggestion is completely get rid of grep(), making it exact match. Both regular expression way to match or partial match makes the function difficult to use programmingly.

Thanks!

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 a pull request may close this issue.

1 participant