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

Introduce different matching modes #15

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@metas-ts
Copy link

metas-ts commented Feb 14, 2019

  • SnapshotMatchRule - introduce a pluggable matching mechanism
  • Specified via SnapshotConfig
  • 3 Implementations:
    • string equality machting, the way it was before
    • Strict matching via JSONAssert
    • Lenient matching via JSONAssert
  • The default behavior is string equality machting
  • Replace raw snapshot string with SnapshotData and SnapshotDataItem
    • used to separate the "real" JSON from the raw snapshot data; without it, JSONAssert failed

AFAIS related #9

PS: @andrebonna i messed up the formatting here and there..if you have eclilpse, maybe publish you formatting rules; then i can import then and reformat it..

metas-ts added some commits Feb 14, 2019

Ignore order of fields in snapshot
* SnapshotMatchRule - introduce a pluggable matching mechanism
* Specified via SnapshotConfig
* 3 Implementations:
  * string equality machting, the way it was before
  * Strict matching via JSONAssert
  * Lenient matching via JSONAssert
* The default behavior is string equality machting
* Replace raw snapshot string with SnapshotData and SnapshotDataItem
  * used to separate the "real" JSON from the raw snapshot data; without it, JSONAssert failed

#9
Ignore order of fields in snapshot
* remove accidentally added license headers

#9
@grimsa
Copy link
Collaborator

grimsa left a comment

I like the fact that matching strategies are now configurable.
I've just recently started using json-snapshot lib and this is something that would definitely be useful.


String rawSnapshot = getRawSnapshot(rawSnapshots);
final SnapshotData snapshots = snapshotFile.getStoredSnapshots();
final SnapshotDataItem snapshotOrNull = snapshots.getItemByNameOrNull(getSnapshotName());

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

Consider returning Optional<SnapshotDataItem>.

On why null values should be avoided.

This comment has been minimized.

@metas-ts

metas-ts Feb 27, 2019

Author

changing it to Optional<SnapshotDataItem>, and then:

	if (snapshot.isPresent()) {
      snapshotMatchRule.match(snapshot.get().getData(), currentObject.getData());
    }
    // Create New Snapshot
    else {
      snapshotFile.push(currentObject);
    }

the article argues against using Optional.get()..i considered doing something with map(), but it seemd much more clusy..wdyt?

import io.github.jsonSnapshot.SnapshotMatchException;

public interface SnapshotMatchRule {
void match(String rawSnapshot, String currentObject) throws SnapshotMatchException;

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

This interface is a nice addition.

A few thoughts:

  1. SnapshotMatchingPolicy or SnapshotMatchingStrategy might be a better name, as it might encompass multiple rules.
  2. Consider moving this interface to the top-level package and leaving all its implementation details (different matching policies) in matchingpolicy or similar package (named to follow the interface). See argument for this packaging strategy
  3. Consider passing SnapshotDataItems into this method. This way it's more flexible, as in the future SnapshotDataItem might hold extra information that would be relevant for matching.

This comment has been minimized.

@metas-ts

metas-ts Feb 27, 2019

Author

doing this

@Value
public class SnapshotData {
@Getter(AccessLevel.NONE)
TreeMap<String, SnapshotDataItem> snapshotDataItems;

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

Consider making this private and initializing in the field declaration, that way constructor would not be required.

This comment has been minimized.

@metas-ts
}

public List<SnapshotDataItem> getItems() {
return new ArrayList<>(snapshotDataItems.values());

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

If the list is not intended to be modified, consider returning Collections.unmodifiableList wrapper

This comment has been minimized.

@metas-ts
return this.name.compareTo(o.name);
}

public static final class IllegalRawDataException extends IllegalArgumentException {

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

Consider removing this exception subclass.

If this exception is unchecked, we don't expect the user to catch it.
If that is the case, then IllegalArgumentException is as informative as IllegalRawDataException.

This comment has been minimized.

@metas-ts

metas-ts Feb 27, 2019

Author

@grimsa
hm..i think the class name IllegalRawDataException makes it more easy to understand what the string we create the exception with is about (e.g. it is not an illegal snapshot name).
Pls let me know what you think..if you still think i should change it to IllegalArgumentException (i don't think it should be a checked one), no problem doing that.

return new SnapshotDataItem(name, data);
}

String name;

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

I'd make the fields private and final for those not familiar with Lombok.
Usually, fields in a class are placed before any methods (even static ones)

This comment has been minimized.

@metas-ts

metas-ts Feb 27, 2019

Author

done (both)

return str.trim();
}

public static SnapshotDataItem ofNameAndData(

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

IMO new SnapshotDataItem(name, data) is just as informative as this method.
It also does not make me wonder if any extra logic hides behind this factory method.

This comment has been minimized.

@metas-ts

metas-ts Feb 27, 2019

Author

@grimsa I prefer such static methods over constructors, but if you like, I'm fine changing it. Pls let me know.

rawSnapshots =
Stream.of(fileContent.toString().split(SPLIT_STRING))
.map(String::trim)
.collect(Collectors.toCollection(TreeSet::new));

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

Retain stream-based solution if possible - they are often more expressive.

This comment has been minimized.

@metas-ts

metas-ts Feb 27, 2019

Author

done (sortof..let's see what you think)

}

try (final FileOutputStream fileStream = new FileOutputStream(file, false)) {
byte[] myBytes = StringUtils.join(rawItems, SPLIT_STRING).getBytes();

This comment has been minimized.

@grimsa

grimsa Feb 15, 2019

Collaborator

myBytes could be constructed outside of try-catch block in a single stream-based expression.

This comment has been minimized.

@metas-ts
@metas-ts

This comment has been minimized.

Copy link
Author

metas-ts commented Feb 18, 2019

Hi @grimsa thank you very much for your review.

Regarding streams, I agree they are often more expressive. Still I'm not a big fan of them (anymore), because they are such a hassle to debug.
However, that's just my five cents, If you prefer them in your project I'm fine with that.

I'm going to change the PR according to your comments, but I'm not sure if I get to it this week.
But it's on my list :-)

EDIT (2019-02-28): done, see commit 7e6261e

Introduce different matching modes
follow most of the advise from #15 (review)
@grimsa note that i didn't yet do the following:
* #15 (comment)
* #15 (comment)

though i have no strong feelings there (see my replies). Pls let me know what you think.

#9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.