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

Fixes and improvements #28

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shahrokh-bahtooei
Copy link
Contributor

This pull request includes a bug fix, an enhancement on fetching pronunciations, and a lot of refactoring. I wish there were some unit tests to ensure the inexistence of any loss in program behavior and reliability. However, whatever I manually tested performed well and improved. Do any refinements in case you found a better organization, more clear structure, improved messages shown to the user, or more meaningful names for variables or functions. I wish I could break the last commit containing a big refactoring into lots of smaller ones, yet the final design was not clear at the beginning and I did lots of editing to come into this committed version. It has taken lots of time from me so far, so I desire it makes us both delighted.

The code intended to show the user 'Potential Matches' when the entry
is not found on Webster's dictionary. The feature is valuable since it
guides the user to the nearest valid words. Still, there is a
mistake in the condition that prevents it to be met at the right
moments. Fix the condition to let the add-on notify users of the
suggested words.
DRY rule says, "Don't repeat yourself." The code will be easier to
read if we develop every logic once and then reuse it.
There are many words in Merriam Webster’s dictionary that derive from
other words but do not include a definition. Webster calls these
words Undefined Run-Ons (UROs). Despite lack of definition, it has
specified part-of-speech for all, and stated pronunciation and
phonetic transcription for considerable numbers of them.

To cover also adding pronunciation and phonetic transcription of
undefined words, we should look for them at URO (Undefined Run-On)
tag of the related entries. This requires us to include potential
entries in our search criteria in due time and expand procedures of
adding vocal pronunciation and phonetic transcription.

To reduce complexity, clean coders prefer modular structure. Refactor
_get_definition() while keeping the solution approach as same as
possible.

Resolves: z1lc#27
@z1lc
Copy link
Owner

z1lc commented Jun 27, 2019

Thank you for submitting this PR with improvements. In its current form, however, this PR is too difficult for me to review -- it changes over half lines of code in the file that holds the majority of the add-on's logic.

In general, it is preferable to have refactoring PRs be separate from new feature PRs -- this way, it's easier to see what's actually new code versus what is just a reorganization. Do you think you could split this PR into 2 or 3 individual pull requests?

@shahrokh-bahtooei
Copy link
Contributor Author

shahrokh-bahtooei commented Jun 27, 2019

I wish I could split the last big commit into some smaller ones, yet currently that I'm using the add-on for memorizing words for my exam, it's not possible for me. If I had enough time, I even developed some test codes prior to refactoring. However, in near four months to my TOEFL exam, I couldn't spend time on coding anymore. This version of the code is the one I'm using for myself, so I wanted to share it with you. So sorry that I could not do you the favor at the moment.

Cheers,

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.

None yet

2 participants