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

Diff demo: lowercase only #567

Closed
wants to merge 3 commits into from
Closed

Conversation

therealryan
Copy link
Collaborator

@therealryan therealryan commented Oct 6, 2023

Extremely important business requirement update for the example app! We should be ignoring character case: return a count of characters after converting all input to lowercase.

We're not going to merge this, it's just to provide a demo of the model-diff function of the execution report.

@therealryan therealryan added the documentation Improvements or additions to documentation label Oct 6, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@therealryan therealryan closed this Oct 6, 2023
@therealryan therealryan mentioned this pull request Oct 6, 2023
@@ -40,7 +40,7 @@ public Map<String, Integer> histogram( String text, String characters ) {

private static Map<String, Integer> histogram( String text, Predicate<Character> test ) {
Map<String, Integer> m = new TreeMap<>();
Optional.ofNullable( text )
Optional.ofNullable( text.toLowerCase() )
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This application change satisfies the new requirement

@@ -28,7 +28,7 @@ public Map<String, Integer> histogram( String text ) {
@Override
public Map<String, Integer> histogram( String text, String characters ) {
LOG.info( "Counting [{}] characters in '{}'", characters, text );
Optional<Set<Character>> queried = Optional.ofNullable( characters )
Optional<Set<Character>> queried = Optional.ofNullable( characters.toLowerCase() )
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sense trying to count upper-case characters any more.
Canny reviewers (and mutation testing) will spot that this change is not exercised by our testing - we don't have any flows where the set of characters to count includes upper-case values.

List<Object> counts = Arrays.asList( " ", 2, "!", 1, "'", 1, "a", 1, "b", 2, "c", 1, "e", 1,
"i", 1, "k", 1, "l", 2 );
String countText = "{\" \":2,\"!\":1,\"'\":1,\"a\":1,"
+ "\"b\":2,\"c\":1,\"e\":1,\"i\":1,\"k\":1,\"l\":2}";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the impact of the change on what gets stored to the DB


List<Object> deletes = valueFill( DELETE,
" ", "!", "H", "d", "l", "r", "w" );
" ", "!", "h", "d", "l", "r", "w" );
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More test-data changes to make our existing test reflect the new behaviour of the system.

.update( HISTOGRAM,
rq( ".+", "HELLO WORLD!" ) ) );

members( flatten( empty, hello, yodel, vowels, shouty ) );
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're also adding a new flow to make it really obvious that upper-case characters are counted as lower-case.

"RESPONSES <-- HISTOGRAM",
"1744EA709AEE2A8718AE343B15C399BC 0009 1.6 KiB" );
"BAEF88E9F3765D7D563B1563C34255C6 0010 1.8 KiB" );
}
Copy link
Collaborator Author

@therealryan therealryan Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've changed a bunch of message content - this diff highlights exactly which services are affected.

"| 0 0.00%│ 0 0.00%|",
"| 0 0.00%│ 1 7.14%|",
"| 0 0.00%│ 1 6.67%|",
"| 0 0.00%│ 0 0.00%|",
"| 0 0.00%│ 0 0.00%|",
"| 0 0.00%│ 0 0.00%|",
Copy link
Collaborator Author

@therealryan therealryan Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of the new flow has shifted the debt histogram about a bit, but note that the "Total Debt" figure has not increased. This indicates that we chose the best possible derivation basis for our new flow.

@therealryan therealryan reopened this Mar 13, 2024
Copy link

sonarcloud bot commented Mar 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant