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

Added isFreightContainerID validator #2144

Merged
merged 7 commits into from
Feb 8, 2023

Conversation

songyuew
Copy link
Contributor

Added isContainerID validator

Check whether a string is an ISO6346 shipping container identification.

https://en.wikipedia.org/wiki/ISO_6346

container_identification_system2

Please review my PR, thanks!

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (147f15a) compared to base (54d330c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2144   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          105       106    +1     
  Lines         2324      2348   +24     
  Branches       586       593    +7     
=========================================
+ Hits          2324      2348   +24     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/lib/isISO6346.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.


// https://en.wikipedia.org/wiki/ISO_6346
const isContainerIdReg = new RegExp('^[A-Z]{3}U[0-9]{7}$');
const isDigit = new RegExp('^[0-9]{1}');
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this could be rewritten to ^[0-9]$, since you are checking against each string character using its index, and that will always be just one character

Also maybe just a matter of taste in general, but this project seems to be generally prefering the use of regular expression literals. as opposed to a constructor function, like you used here.
So for the sake of style consistency, I would probably also suggest the following change:

new RegExp('^[0-9]{1}')-> const isDigit = /^[0-9]$/
(same applies to the isContainerIdReg regexp in line 4)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Panagiotis,

Thank you very much for pointing it out! I have updated the corresponding lines.


export default function isContainerID(str) {
assertString(str);
str = str.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same concerns about this, as I have mentioned in your other PR:
#2143

what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

You are right @pano9000 -- I don't think we should be doing cleaning behind the validators, let the users do their own cleaning. @songyuew pls update.

README.md Outdated
@@ -102,6 +102,7 @@ Validator | Description
**isBoolean(str [, options])** | check if a string is a boolean.<br/>`options` is an object which defaults to `{ loose: false }`. If loose is is set to false, the validator will strictly match ['true', 'false', '0', '1']. If loose is set to true, the validator will also match 'yes', 'no', and will match a valid boolean string of any case. (eg: ['true', 'True', 'TRUE']).
**isBtcAddress(str)** | check if the string is a valid BTC address.
**isByteLength(str [, options])** | check if the string's length (in UTF-8 bytes) falls in a range.<br/><br/>`options` is an object which defaults to `{min:0, max: undefined}`.
**isContainerID(str)** | check if the string is an ISO6346 shipping container identification.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name might be a bit too unspecific, as "container ID" feels a bit too generic, e.g. just google it, brought up several hits, like e.g. "docker container ID", "Windows Drivers Container ID", etc.

So I would probably say it makes sense to rename this?

Options could be maybe:

  • isShippingContainerID
  • isISO6346ContainerID (that one does look ugly to be honest :-D)
  • ?

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We already have some ISO standards so a simple isISO6346 could also suffice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it to isISO6346, for consistency with other validators.

@pano9000
Copy link
Contributor

thank you for the PR :-)
Interesting to see what kind of numbers there are out there in the world :-D

@songyuew
Copy link
Contributor Author

thank you for the PR :-) Interesting to see what kind of numbers there are out there in the world :-D

Haha thank you Panagiotis for reviewing my PR once again.

@profnandaa
Copy link
Member

This was also interesting to me, learned something new! Thanks for the PR!

profnandaa
profnandaa previously approved these changes Jan 20, 2023
Copy link
Member

@profnandaa profnandaa 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! 🎉

@profnandaa
Copy link
Member

QQ, do you want to add isFreightContainerID too as an alias to isISO6346? I recommend adding Freight to it for clarity. @songyuew

@profnandaa profnandaa added the 🧹 needs-update For PRs that need to be updated before landing label Jan 20, 2023
@songyuew
Copy link
Contributor Author

Hi Nandaa,

Do you mean adding a new file with the name of isFreightContainerID, or simply just creating a new entry in the readme?

@pano9000
Copy link
Contributor

hi guys,

one tiny thing I'd like to still see adressed is my comment above regarding the use of str.trim()
#2144 (comment)

@profnandaa
Copy link
Member

Hi Nandaa,

Do you mean adding a new file with the name of isFreightContainerID, or simply just creating a new entry in the readme?

@songyuew -- not really a new file, just within the same file, exporting the isFreightContainerID = isISO6346; and exporting both.

@songyuew
Copy link
Contributor Author

Hi Nandaa,
Do you mean adding a new file with the name of isFreightContainerID, or simply just creating a new entry in the readme?

@songyuew -- not really a new file, just within the same file, exporting the isFreightContainerID = isISO6346; and exporting both.

@profnandaa @pano9000 Alias added and sanitization removed, plz kindly review, thanks!

pano9000
pano9000 previously approved these changes Feb 5, 2023
@pano9000 pano9000 removed the 🧹 needs-update For PRs that need to be updated before landing label Feb 5, 2023
@profnandaa
Copy link
Member

My bad, I don't know how we missed this one, should have gone into the previous release. I'll queue it up for the next one.

profnandaa
profnandaa previously approved these changes Feb 5, 2023
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM. If you can fix the m/c, I'll merge it straight away.

@profnandaa profnandaa added mc-to-land Just merge-conflict standing between the PR and landing. ✅ LGTM labels Feb 5, 2023
@songyuew songyuew dismissed stale reviews from profnandaa and pano9000 via b76305e February 6, 2023 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 first-pr ✅ LGTM mc-to-land Just merge-conflict standing between the PR and landing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants