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 bugs to ignore Capital letter at the beginning of the Sentence #6

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

donganzh-zz
Copy link
Contributor

Hi, @azu
As I was using this rule on my documents, I found a bug that if there is a capital letter at the beginning of the sentence and there is an explanation to an acronym right after it, the rule won't extract the right acronym for the comparison. Here is an example:

In the Amazon Web Service, it can be short as AWS.

In this case, previously the rule will extract IAWS to compare with AWS because "the" is an acronymJoiningWords, which generates an error message.

In order to fix it, I first reverse the AcronymCreater array and loop using reduce and add the first character to beginning on every iteration and finally join on every iteration and push to result array

{"S", "WS", "AWS", "IAWS"}

Then check this array using isWordSatidfy method and push the good ones to expandedAcronymList to compare with AWS.

I added this test case in the textlint-rule-unexpanded-acronym-test.js and it passed all the previous test as well.

I also update the array-includes package to the latest version.

Cheers!

Copy link
Member

@azu azu left a comment

Choose a reason for hiding this comment

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

Can you add test case for this bug?
We want to prevent regression.

src/word-utils.js Outdated Show resolved Hide resolved
src/word-utils.js Outdated Show resolved Hide resolved
src/word-utils.js Outdated Show resolved Hide resolved
src/word-utils.js Outdated Show resolved Hide resolved
src/textlint-rule-unexpanded-acronym.js Outdated Show resolved Hide resolved
*/
export function expandWordsToAcronym(words) {
//XMLHttpRequest -> XHR
if (words.length === 1) {
return expandOneWordToAcronym(words[0]);
Copy link
Member

@azu azu Aug 4, 2019

Choose a reason for hiding this comment

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

It is good interface that always return an array.

return [expandOneWordToAcronym(words[0]))

This change will remove unnessary Array.isArray code from https://github.com/textlint-rule/textlint-rule-unexpanded-acronym/pull/6/files#diff-8c133963a70d07eee0bd3b0a4e71f38eR72

                    var acronyms = acronymCreator.extractAcronym();
-                    //if the acronyms is a string i.e. only one acronym
-                    if(!Array.isArray(acronyms)){
-                        if (isWordSatisfy(acronyms)) {
-                            expandedAcronymList.push(acronyms);
-                        }
-                    //if the acronyms is an array of acronym, we need - to extract the satisfied ones
-                    }else{
                        acronyms.forEach(acronym => {
                            if (isWordSatisfy(acronym)) {
                                expandedAcronymList.push(acronym);
                            }
                        });
-                    }

* @returns {string}
* @example XMLHttpRequest -> XHR
* @example World Health Organization -> WHO
* @param words (array)
Copy link
Member

Choose a reason for hiding this comment

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

It is not JSDoc.

Suggested change
* @param words (array)
* @param {string[]} words

src/word-utils.js Show resolved Hide resolved
@donganzh-zz
Copy link
Contributor Author

Changes fixed

@donganzh-zz
Copy link
Contributor Author

@azu hello, could you please review my changes? thanks

@parkas2018
Copy link

I have the following sentence where the acronym "AWS" is being reported as unexpanded. Will this PR fix this issue?

This topic describes the process to setup this tool, which you can use for deploying the application on either Azure or Amazon Web Services (AWS)

@amimas
Copy link

amimas commented Apr 27, 2020

Hello, any update on this PR?

Copy link
Member

@azu azu left a comment

Choose a reason for hiding this comment

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

LGTM.

Sorry to delay.

@azu azu merged commit 7d23982 into textlint-rule:master Apr 29, 2020
@azu
Copy link
Member

azu commented Apr 29, 2020

I've published https://github.com/textlint-rule/textlint-rule-unexpanded-acronym/releases/tag/1.2.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants