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

Explicitly list out printable ASCII characters #257

Merged
merged 4 commits into from
Oct 31, 2020

Conversation

ianyong
Copy link
Member

@ianyong ianyong commented Oct 31, 2020

Changes:

  • Remove the abbr HTML tag.
    • Explicitly list out all printable ASCII characters instead.
  • Update constraints messages for Title and Category to be more informative.
  • Add tests to ensure that only printable ASCII characters are allowed for Title and Category.

Addresses #131 (comment).

@ianyong ianyong added type.documentation 🧻 Improvements or additions to documentation priority.medium 🥈 Todo for current iteration labels Oct 31, 2020
@ianyong ianyong added this to the v1.4 milestone Oct 31, 2020
@ianyong ianyong requested a review from a team October 31, 2020 08:11
Also update the constraints messages to be more descriptive.
@codecov-io
Copy link

Codecov Report

Merging #257 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #257   +/-   ##
=========================================
  Coverage     71.58%   71.58%           
  Complexity      897      897           
=========================================
  Files           141      141           
  Lines          2819     2819           
  Branches        330      330           
=========================================
  Hits           2018     2018           
  Misses          682      682           
  Partials        119      119           
Impacted Files Coverage Δ Complexity Δ
..._cs2103_w16_3/finesse/model/category/Category.java 81.81% <ø> (ø) 6.00 <0.00> (ø)
..._cs2103_w16_3/finesse/model/transaction/Title.java 90.90% <ø> (ø) 8.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a22d7ea...0e3c7b9. Read the comment docs.

wltan
wltan previously approved these changes Oct 31, 2020
Copy link
Member

@wltan wltan left a comment

Choose a reason for hiding this comment

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

LGTM, however do note that the issue is currently autolinked to close #131, which might not be intended.

yongping827
yongping827 previously approved these changes Oct 31, 2020
Copy link

@yongping827 yongping827 left a comment

Choose a reason for hiding this comment

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

LGTM

@ianyong ianyong dismissed stale reviews from yongping827 and wltan via ca5c9a4 October 31, 2020 10:54
Copy link
Member

@wltan wltan left a comment

Choose a reason for hiding this comment

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

LGTM!

@siddarth2824 siddarth2824 self-requested a review October 31, 2020 15:41
Copy link

@siddarth2824 siddarth2824 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yongping827 yongping827 merged commit 9e37a56 into master Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.medium 🥈 Todo for current iteration type.documentation 🧻 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants