Allow dashes in author name #42 by yamgent · Pull Request #43 · se-edu/collate

yamgent · 2016-12-09T06:03:18Z

Fixes #42.

Ready for review.

The new files for test case for 'author name with dashes' caused a change in the overall statistics.

HTML comment end tags contain dashes, and dashes are no longer forbidden characters.

The new files for test case for 'author name in HTML files' caused a change in the overall statistics.

riwu · 2016-12-09T08:07:30Z

Should leading/trailing dashes in names be removed? eg. "--author-name -" to "author-name"
If so, this would do the job: master...riwu:dashInName

damithc · 2016-12-09T10:06:36Z

@sebastianquek for your review please.

sebastianquek · 2016-12-10T08:22:09Z

Thanks @yamgent for your contributions!

Background

Looking at the various use cases, the author tag can be followed by several variants of author names:

// @@author A0123456Z
// @@author generated
// @@author A0123456Z-unused
// @@author A0123456Z-reused


/* @@author A0123456Z */

The current implementation allows for spaces in names, and when we allow dashes as well, we will need to handle edge cases such as in point 6 (as @yamgent has found). However, the fix is very specific to -->, and may be adding unnecessary complexity to the codebase.

Given that we currently restrict author names to only alphanumeric characters and dashes as in the above examples, I propose that we remove the feature that allows for spaces in names.

Proposed implementation

One approach is to extract the author's name according to a regex expression. In a very verbose manner: ^[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9].

The ^ indicates the beginning of the sentence,
the 1st matching character set ensures the author's name has to start with an alphanumeric character,
the 2nd set allows any number of alphanumeric characters and -,
the 3rd set is similar to the 1st set.

This ensures that author names that start and/or end with non-alphanumeric characters are illegal (as @riwu has mentioned). Furthermore, alphanumeric characters can be interleaved with - characters (e.g. long-author-name).

Any thoughts/ideas?

damithc · 2016-12-10T09:03:16Z

I'm ok with not allowing spaces in author name.

sebastianquek

Remove ability to have whitespace in author names, use regex that accepts and extracts the valid author names.

This streamlines the extraction of the author name from an author tag as special rules (HTML comments) are not required anymore.

This reverts commit de643b6.

Regex now extracts name, instead of looking for forbidden characters.

Now spaces in author's name are forbidden.

sebastianquek · 2016-12-10T12:07:35Z

src/main/java/backend/Logic.java


-    private static final String REGEX_NEITHER_ALPHANUMERIC_NOR_SPACE_NOR_DASH =
-        "[^ a-zA-Z0-9\\-]+";
+    private static final String REGEX_AUTHOR_NAME_FORMAT = "(^[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]).*";


The .* at the end will match any number of characters that are not line breaks! Do test it with the new test file you created (testFileHtml.html).

sebastianquek · 2016-12-10T12:26:29Z

One last thing, @yamgent could you add test cases for author names that start and/or end with -?

yamgent · 2016-12-10T12:48:59Z

@sebastianquek Okay sure, am working on it.

riwu · 2016-12-10T13:04:30Z

src/main/java/backend/Logic.java


-    private static final String REGEX_NEITHER_ALPHANUMERIC_NOR_SPACE =
-        "[^ a-zA-Z0-9]+";
+    private static final String REGEX_AUTHOR_NAME_FORMAT = "(^[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])";


Is it intended to fail if author string doesn't start with alphanumeric?
Removing '^' from the regex would simply strip leading non-alphanumeric instead of failing.

You're right, if we allow @@author matric-num----, which results with the author name being matric-num, @@author --matric-num---- and @@author !@%$&*^matric-num likewise should not fail.

I fixed it in 769d213.

yamgent · 2016-12-10T13:23:17Z

@sebastianquek I added test cases for the dashes, do take a look.

@riwu thanks for your input!

sebastianquek · 2016-12-10T13:44:51Z

Awesome! 👍
As an exercise about equivalence partitions and their associated boundary values, you can also try adding additional test cases. Recall that any number of non-alphanumeric characters before and after the author's name should not cause a failure. 😄

yamgent · 2016-12-11T12:06:08Z

@sebastianquek Few more test cases added

sebastianquek · 2016-12-13T02:05:20Z

LGTM 👍

yamgent added 7 commits December 9, 2016 12:51

Add test case for dashes in author name

7a01c99

Fix logic stripping out dashes in author names

ccac730

Fix failing test cases

f87aba9

The new files for test case for 'author name with dashes' caused a change in the overall statistics.

Replace comment with refactored method

fb6ffb2

Add test case for author tags in HTML

c3869c2

Fix HTML comment end tags being retained in author names

de643b6

HTML comment end tags contain dashes, and dashes are no longer forbidden characters.

Fix failing test cases

69db5d7

The new files for test case for 'author name in HTML files' caused a change in the overall statistics.

damithc assigned sebastianquek Dec 10, 2016

sebastianquek requested changes Dec 10, 2016

View reviewed changes

sebastianquek mentioned this pull request Dec 10, 2016

A user can demarcate the lines of code that he wrote #1

Closed

yamgent added 3 commits December 10, 2016 19:00

Revert "Fix HTML comment end tags being retained in author names"

84d2185

This reverts commit de643b6.

Change regex to extract name instead

bd3df79

Regex now extracts name, instead of looking for forbidden characters.

Fix failing test cases

62de64e

Now spaces in author's name are forbidden.

sebastianquek reviewed Dec 10, 2016

View reviewed changes

Fix regex matching trailing characters

a3d6d96

sebastianquek approved these changes Dec 10, 2016

View reviewed changes

riwu reviewed Dec 10, 2016

View reviewed changes

yamgent added 3 commits December 10, 2016 21:10

Add test case for starting and ending dashes

5bc6b66

Fix regex failing when starting with dash

769d213

Add more test cases for starting and ending dashes

ea99ca3

yamgent added 2 commits December 11, 2016 09:10

Add more tests for non-alphanumeric cases

c723394

Refactor test helper methods

9c90c57

sebastianquek merged commit 14cb274 into se-edu:master Dec 13, 2016

Conversation

yamgent commented Dec 9, 2016

Uh oh!

riwu commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Dec 9, 2016

Uh oh!

sebastianquek commented Dec 10, 2016

Background

Proposed implementation

Uh oh!

damithc commented Dec 10, 2016

Uh oh!

sebastianquek left a comment

Choose a reason for hiding this comment

Uh oh!

sebastianquek Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianquek commented Dec 10, 2016

Uh oh!

yamgent commented Dec 10, 2016

Uh oh!

riwu Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

sebastianquek Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

yamgent Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

yamgent commented Dec 10, 2016

Uh oh!

sebastianquek commented Dec 10, 2016

Uh oh!

yamgent commented Dec 11, 2016

Uh oh!

sebastianquek commented Dec 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

riwu commented Dec 9, 2016 •

edited

Loading

sebastianquek Dec 10, 2016 •

edited

Loading