Skip to content

fix: fix the problem that nested list cannot be parsed correctly #2136

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

juzi214032
Copy link
Contributor

close #2103

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 87.161% when pulling fc2d057 on juzi214032:fix/issue/2103 into ac2f383 on mybatis:master.

@juzi214032
Copy link
Contributor Author

juzi214032 commented Dec 15, 2020

Fix the problem of not being able to parse nested lists by parsing the parentheses in reverse order.
Request a review, thanks! @hazendaz @harawata

@juzi214032
Copy link
Contributor Author

@harawata @hazendaz TBR

@juzi214032
Copy link
Contributor Author

@harawata Is it possible to merge this pr

@juzi214032
Copy link
Contributor Author

@harawata TBR. Thanks!

@juzi214032
Copy link
Contributor Author

@harawata @hazendaz
Please ask whether this project currently accepts pr it, I see a lot of pr are not merged, I am trying to join the project to develop some features. But if there is no one to maintain and review code for this project, I will give up

@jeffgbutler
Copy link
Member

@juzi214032 calm down. We are all volunteers here and doing this work on our own time. Yes, we accept PRs, but they need to be carefully reviewed before being merged. Someone will take a look at this PR eventually.

@juzi214032
Copy link
Contributor Author

@jeffgbutler ok, sorry. I just want to make sure this project is still in development

@harawata
Copy link
Member

Hello @juzi214032 ,

This change breaks the contract of PropertyTokenizer because the following test fails.

@Test
void testIndexedName() {
  PropertyTokenizer tokenizer = new PropertyTokenizer("simpleName[2]");
  assertEquals("simpleName", tokenizer.getName());
  assertEquals("simpleName[2]", tokenizer.getIndexedName()); // actual = "simpleName"
  assertNull(tokenizer.getChildren());
  assertEquals("2", tokenizer.getIndex());
  assertFalse(tokenizer.hasNext());
}

However, it does not break any existing tests which means either 1) getIndexName() actually is unnecessary or 2) there is not enough test.
We should find out which the case is, but either way, this PR may not be the right fix.

@juzi214032
Copy link
Contributor Author

@harawata thanks review, i will try to fix it

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.

mybatis can not parse #{list[0][0]} correctly
4 participants