Skip to content

Fix many typos #2802

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Sep 23, 2025
Merged

Fix many typos #2802

merged 11 commits into from
Sep 23, 2025

Conversation

@jeis4wpi
Copy link
Contributor

@jeis4wpi jeis4wpi commented Sep 7, 2025

is messsage a concern on this line of code ?

@somiaj
Copy link
Contributor

somiaj commented Sep 7, 2025

I would suggest renaming your commits to not be questions, but state what they do.

drgrice1
drgrice1 reviewed Sep 7, 2025
View reviewed changes
lib/WeBWorK/Localize/webwork2.pot Outdated

#: /opt/webwork/webwork2/lib/WeBWorK/ContentGenerator/Problem.pm:1483
msgid "ATTEMPT NOT ACCEPTED -- Please submit answers again (or request new version if neccessary)."
msgid "ATTEMPT NOT ACCEPTED -- Please submit answers again (or request new version if necessary)."
Copy link
Member

Choose a reason for hiding this comment

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

The webwork2.pot file should not be directly modified. It is generated by the bin/dev_scripts/update-localization-files script. Once the string is corrected in the code, then this will be fixed when that script is run again.

@drgrice1
Copy link
Member

drgrice1 commented Sep 7, 2025

The line in the tagwidget.js file is a typo, but not a major concern. It would only happen if the taxonomy file failed to be downloaded. Note that this is only something that admin users use in any case, and even then not much.

@jeis4wpi jeis4wpi closed this Sep 8, 2025
@drgrice1
Copy link
Member

drgrice1 commented Sep 8, 2025

This was a fine pull request. Why did you close it?

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 8, 2025 via email

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 8, 2025

I would like to remove all typos if that is okay

@jeis4wpi jeis4wpi reopened this Sep 8, 2025
@somiaj
Copy link
Contributor

somiaj commented Sep 8, 2025

Just edit your commits and force push your branch to address the concerns raised here. It overall looks good.

@drgrice1
Copy link
Member

drgrice1 commented Sep 8, 2025

What exactly do you mean by "remove all typos"? Do you mean remove all of the fixes of typos from this pull request, or do you mean to make the pull request fix all typos in the code, or do you mean something else entirely?

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 8, 2025 via email

@drgrice1
Copy link
Member

drgrice1 commented Sep 8, 2025

If you find other typos, you can either add them to this pull request or as you said, create another pull request.

Note that you should not fix typos in any of the *.pot or *.po files in the lib/WeBWorK/Localize directory, since those are generated files and generally should not be directly edited.

Of course if you make a change that shouldn't be, then we will comment on it here (or in another pull request you make).

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 8, 2025 via email

@pstaabp pstaabp changed the title is this a concern ? Fix many typos Sep 9, 2025
@Alex-Jordan
Copy link
Contributor

This looks good, thanks. Except it would be better to remove the change to lib/WeBWorK/Localize/webwork2.pot from the PR. If you could remove that, I imagine we could quickly merge this now that it's been reviewed.

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 12, 2025 via email

drgrice1
drgrice1 reviewed Sep 14, 2025
View reviewed changes
bin/dev_scripts/run-perltidy.pl
Comment on lines 68 to 69
exit $err if $err;
die "The -pro option is not suppored by this script.\n" if defined $options{profile};
die "The -pro option is not supported by this script.\n" if defined $options{profile};
Copy link
Member

Choose a reason for hiding this comment

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

Due to the addition of a letter on line 69, another space is needed on line 68 to make the ifs line up. That is why the perltidy action is failing.

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 14, 2025 via email

@drgrice1
Copy link
Member

Fix the spacing. The spelling error should be fixed.

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 14, 2025 via email

@drgrice1
Copy link
Member

Ideally you would have your text editor set up to automatically run perltidy anytime you save a Perl file. That way you don't have to think about it.

@jeis4wpi
Copy link
Contributor Author

jeis4wpi commented Sep 14, 2025 via email

drgrice1
drgrice1 reviewed Sep 15, 2025
View reviewed changes
lib/WeBWorK/Utils.pm Outdated
Usage: C<cryptPassword($clearPassword)>
Returns the crypted form of C<$clearPassword> using a random 16 character
Returns the encrypted form of C<$clearPassword> using a random 16 character
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually a typo. Crypted is a slangy way of saying encrypted. Also, the Perl crypt method is used to encrypt the password, and so crypted is the past tense of that method name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for fixing.

Alex-Jordan
Alex-Jordan approved these changes Sep 15, 2025
View reviewed changes
Copy link
Contributor

@Alex-Jordan Alex-Jordan left a comment

Choose a reason for hiding this comment

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

This looks good, although I have no opinion on whether "encrypted" should go back to "crypted". I'm marking as approved so we can get this in, and any future typo fixes could be on a smaller PR.

@jeis4wpi
Copy link
Contributor Author

Since Perl crypt is understood by developers, will it also be understood by Faculty and students?

@drgrice1
Copy link
Member

The place where this is is only for developers. So your question does not pertain to that situation.

@jeis4wpi
Copy link
Contributor Author

Thank you for the perl crypt education. Do I need to revert any other files?

@drgrice1
Copy link
Member

Everything else looks good.

pstaabp
pstaabp approved these changes Sep 16, 2025
View reviewed changes
Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

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

I'm on the fence on the "crypted" versus "encrypted" on the one line, but everything else looks good to me.

@drgrice1
Copy link
Member

On the crytped usage: Note that crypted is also used in the POD in lib/WeBWorK/Utils.pm for the cryptPassword method. The method has always been named cryptPassword and not encryptPassword. The variable in the method has always been $cryptPassword. The perl method is crypt and no encrypt method exists. A related script is bin/crypt_passwords_in_classlist.pl.

The word crypt and not encrypt is consistent throughout the entire code base as pertains to passwords. Do you want to change all of that including the Perl method? Once you have accomlished that then you can change the word in lib/WeBWorK/Authen.pm.

@dlglin
Copy link
Member

dlglin commented Sep 16, 2025

I think that it should remain as crypt, as that is the name of the underlying function.

@drgrice1 drgrice1 merged commit d712732 into openwebwork:develop Sep 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drgrice1 drgrice1 drgrice1 left review comments

@pstaabp pstaabp pstaabp approved these changes

@Alex-Jordan Alex-Jordan Alex-Jordan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jeis4wpi @somiaj @drgrice1 @Alex-Jordan @dlglin @pstaabp