Skip to content
This repository has been archived by the owner on Apr 6, 2024. It is now read-only.

Restrict all command input to ASCII #148

Merged
merged 9 commits into from
Nov 2, 2023

Conversation

conradsoon
Copy link

This fixes #147

@conradsoon conradsoon added this to the v1.4 milestone Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (700c007) 82.75% compared to head (f7e1736) 82.78%.
Report is 14 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #148      +/-   ##
============================================
+ Coverage     82.75%   82.78%   +0.02%     
- Complexity      754      755       +1     
============================================
  Files            99       99              
  Lines          2308     2312       +4     
  Branches        258      259       +1     
============================================
+ Hits           1910     1914       +4     
  Misses          333      333              
  Partials         65       65              
Files Coverage Δ
.../seedu/address/logic/parser/AddressBookParser.java 90.32% <100.00%> (+1.43%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@conradsoon conradsoon modified the milestones: v1.4, v1.3b Nov 2, 2023
Copy link

@lordidiot lordidiot left a comment

Choose a reason for hiding this comment

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

Some small changes then let's merge this

@Test
public void parseCommand_allAsciiInputAllowed() {
AddressBookParser parser = new AddressBookParser();
for (char c = '!'; c <= '~'; c++) {

Choose a reason for hiding this comment

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

Shouldn't this start with space?

Suggested change
for (char c = '!'; c <= '~'; c++) {
for (char c = ' '; c <= '~'; c++) {

Copy link
Author

Choose a reason for hiding this comment

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

Space gets trimmed and gets a different error. Starting with '!' is correct.

Copy link
Author

Choose a reason for hiding this comment

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

Changed test case in #f7e1736

Copy link

@limjunxian1 limjunxian1 left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the quick changes and writing a neater test case for this!

Copy link

@lordidiot lordidiot left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! This should help us handle an entire class of issues 😲

@conradsoon conradsoon merged commit 3394a5f into AY2324S1-CS2103T-T13-2:master Nov 2, 2023
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict input to ASCII characters app-wide
3 participants