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

issue #87 cupsfilter showing particular errors. #88

Merged
merged 4 commits into from Feb 11, 2021

Conversation

surajkulriya
Copy link
Contributor

@surajkulriya surajkulriya commented Feb 4, 2021

Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

I generally approve of the changes. See my comments about coding style/indentation and the use of the "_" macro for localization.

@@ -221,8 +227,10 @@ main(int argc, /* I - Number of command-line args */
i ++;
if (i < argc && !infile)
infile = argv[i];
else
usage(opt);
else{
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: CUPS puts open braces on their own line (not K&R), uses (traditional) 8 column tabs, and two spaces of indentation, e.g.:

else
{
  _cupsLanfPrintf(stderr, _("%s: Error - ..."), argv[0]);
  usage(NULL);
}

@@ -184,9 +184,11 @@ main(int argc, /* I - Number of command-line args */
i ++;
if (i < argc)
num_options = cupsParseOptions(argv[i], num_options, &options);
else
else{
_cupsLangPrintf(stderr,"%s: Error - expected option after \"-a\" option.",argv[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Messages need to use the _("string") macro in order to get scanned by gettext (which we use to build the message catalog).

@michaelrsweet michaelrsweet self-assigned this Feb 5, 2021
@michaelrsweet michaelrsweet added enhancement New feature or request priority-low labels Feb 5, 2021
Comment on lines 249 to +253
else
usage(opt);
{
_cupsLangPrintf(stderr,_("%s: Error - expected source MIME type after \"-i\" option."),argv[0]);
usage(NULL);
}
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 don't exactly know why its showing like this in github. I am using vscode editor for editing files. else statement and opening brace { are being shown in vertical line there.

@surajkulriya
Copy link
Contributor Author

@michaelrsweet I am sorry for the mess I created. By the way is there still a mistake in indentation. Like I already told you its being shown differently in github, in vscodes it's looking like this:=

Screenshot from 2021-02-07 22-05-00

If you want you can close this pull request, I will make another one in which I will make all these change in a single commit to sort all this.
Thanks.

@michaelrsweet
Copy link
Member

@surajkulriya It's OK, take your time. The final merge will be clean when it happens.

FWIW, I was hoping VS Code might be a common editor to use across platforms but I've never been able to get the tab settings to work the way I want. For Linux, GNOME's editor is OK for simple stuff, and for macOS I've been using BBEdit for years in addition to Xcode (whose editor only seems to get worse with every new release...)

@surajkulriya
Copy link
Contributor Author

@michaelrsweet I found this.
The indentation issue that's arising is because github assumes tabsize to be 8 spaces.
append ?ts=2 or ?ts=4 at the end of the url to change the default setting. Something like this https://github.com/OpenPrinting/cups/pull/88/files?ts=2.
So basically it's not an indentation issue.
Is PR mergeable now. Do tell me if there are still some more changes to make.
Thanks

@surajkulriya
Copy link
Contributor Author

@michaelrsweet I have made all the changes you had requested. Please do tell me if I am missing out something or if you want me to make some more changes as well.
Any other particular reason why you still have not merged this PR.
Thanks

@michaelrsweet
Copy link
Member

@surajkulriya Sorry, I've been busy with PWG meetings and am just getting some time to look at this again. You changes look good and I'll fix any indentation problems (if any) once I've merged. Thanks!

@michaelrsweet michaelrsweet merged commit 3da875f into OpenPrinting:master Feb 11, 2021
michaelrsweet added a commit that referenced this pull request Feb 11, 2021
Update message catalogs for new messages.

Update changelog.
@michaelrsweet
Copy link
Member

[master 1feec78] Fix indentation for cupsfilter error messages (Issue #88)

@surajkulriya
Copy link
Contributor Author

Thanks @michaelrsweet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants