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

Fix special char usage in cmd line args for Rate Limiting the PRs #776

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

tarishij17
Copy link
Contributor

No description provided.

@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): tok-sfci343 <t***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

@github-actions github-actions bot added the tests Improvements / maintenance to tests label Jan 31, 2023
@@ -75,7 +75,7 @@ public RateLimiter() {
* @see net.sourceforge.argparse4j.inf.Namespace Namespace
*/
public static RateLimiter getInstance(Namespace ns) {
String rateLimitPRCreation = ns.get(Constants.RATE_LIMIT_PR_CREATION);
String rateLimitPRCreation = ns.get((Constants.RATE_LIMIT_PR_CREATION).replace("-","_"));
Copy link
Member

Choose a reason for hiding this comment

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

I think the better thing is to use the underscore in the constant string and add a unit test to validate that no char is used in ArgumentParser that will be replaced automatically.
Also, we need to test the changes once again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the error that we are hitting if we don't make this change? Can we change the documentation is error out if invalid chars are passed?

Copy link
Member

Choose a reason for hiding this comment

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

'-' is replaced by '_' by argparse4j ArgumentParser while parsing arguments. so "random-arg val" will be set to random_arg=val in the namespace.
What we are purposing here is

  1. change the argument using - to use _
  2. add a unit test to throw the error if someone uses it unintentionally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, so users won't see any difference, it is just how we parse internally?

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #776 (28be9ba) into main (890e120) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #776   +/-   ##
=========================================
  Coverage     84.40%   84.40%           
  Complexity      374      374           
=========================================
  Files            27       27           
  Lines          1244     1244           
  Branches        164      164           
=========================================
  Hits           1050     1050           
  Misses          157      157           
  Partials         37       37           
Impacted Files Coverage Δ
...esforce/dockerfileimageupdate/utils/Constants.java 100.00% <ø> (ø)

Copy link
Member

@jeetchoudhary jeetchoudhary left a comment

Choose a reason for hiding this comment

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

LGTM

@jeetchoudhary jeetchoudhary merged commit 02fcadf into salesforce:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:missing tests Improvements / maintenance to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants