Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Issue #127, JSON in alias filter is unnecessarily escaped. Changed Strin... #128

Merged
merged 2 commits into from
Jun 17, 2014
Merged

Issue #127, JSON in alias filter is unnecessarily escaped. Changed Strin... #128

merged 2 commits into from
Jun 17, 2014

Conversation

igor-kupczynski
Copy link
Contributor

Example solution to Isuse #127: Changed String to JsonObject in AliasMapping actions.

…ring to JsonObject in AliasMapping actions.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9aca3ed on puszczyk:Issue#127 into * on searchbox-io:master*.

@kramer
Copy link
Member

kramer commented Jun 16, 2014

All looks great except the AliasFilters class which I believe does not really fit the code style of the project (utility class for test data).

@igor-kupczynski
Copy link
Contributor Author

Agree, I haven't seen other cases like this in the tests. What is the preferred approach though? Copy-paste into the test classes or to have a common superclass with the content as a field? Or something else maybe?

@kramer
Copy link
Member

kramer commented Jun 17, 2014

Copy pasting would be the preferred approach for tests, as (imho) using base/utility classes for test purposes mostly decreases readability and any gain from them can be ignored since the purpose is testing.

@igor-kupczynski
Copy link
Contributor Author

OK, I've changed the code. Please take a look :-)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ca39394 on puszczyk:Issue#127 into * on searchbox-io:master*.

kramer pushed a commit that referenced this pull request Jun 17, 2014
Issue #127, JSON in alias filter is unnecessarily escaped. Changed Strin...
@kramer kramer merged commit d80c6ba into searchbox-io:master Jun 17, 2014
@kramer kramer added the bug label Jul 10, 2014
@kramer kramer added this to the 0.1.2 milestone Jul 10, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants