teragrep / pth_06

Teragrep Datasource for Apache Spark
GNU Affero General Public License v3.0
0 stars 5 forks source link

refactor walkers #76

Closed elliVM closed 1 week ago

elliVM commented 2 weeks ago
eemhu commented 2 weeks ago

Should there be unit tests for each of the new condition objects?

elliVM commented 2 weeks ago

added missing tests, IndexStatementCondition test will be updated in a later PR with bloom filter changes.

elliVM commented 2 weeks ago

Refactored tests they should be cleaner now @eemhu

eemhu commented 2 weeks ago

I think the changes look good, however I think object equality tests should be added. Also not 100% sure about the use of Optional, it might be considered better practice to use some sort of "empty" condition object instead.

elliVM commented 1 week ago

Added override for equals(Object o) method for conditions and added equality tests. Refactored Optional to use jooq noCondition.

eemhu commented 1 week ago

Good changes, however I would separate them into equality and non-equality tests to make it more clearer. Each test should test only one thing

Also, you could check for this == other as the first statement in equals() but I'm not sure if it does make a meaningful difference.

elliVM commented 1 week ago

No harm in checking for this == other so added and split tests into equalsTest and notEqualsTest