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

Simplify code #7

Merged
merged 8 commits into from
Jul 4, 2021
Merged

Simplify code #7

merged 8 commits into from
Jul 4, 2021

Conversation

laurayco
Copy link

@laurayco laurayco commented Jul 2, 2021

So I was looking about the source code and noticed it was more verbose than it needed to be, so I cleaned it up. This was just an update of syntax and removing unnecessary Promise hell - everything should work as it did before, but there aren't any tests defined that I could tell - I will probably work on writing tests when I next have the time.

edit: It's worth pointing out that this re-write is still arguably more verbose than it needs to be - you could remove the word "async" from all of the functions and it would be the same since await is never used, but in my subjective opinion async keyword is a good way to indicate via code that a function returns a promise.

Copy link
Member

@kaimoe kaimoe left a comment

Choose a reason for hiding this comment

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

Good catch! You're right that there was no real reason for the promise statements to be used in such a way, either I had something different in mind when I first wrote the functions or I just didn't understand promises as well at the time. I'll be making the changes I'm suggesting below myself and doing some manual tests before merging everything.

Just one thing of note, in the project's ESLint file there is a rule instructing that semicolons shouldn't be used. I know it's a matter of preference but I'll be removing the ones you added for consistency.

lib/character.js Outdated Show resolved Hide resolved
lib/character.js Outdated Show resolved Hide resolved
lib/character.js Show resolved Hide resolved
lib/character.js Outdated Show resolved Hide resolved
lib/data.js Outdated Show resolved Hide resolved
lib/pvpteam.js Outdated Show resolved Hide resolved
lib/pvpteam.js Outdated Show resolved Hide resolved
lib/pvpteam.js Outdated Show resolved Hide resolved
lib/search.js Outdated Show resolved Hide resolved
lib/character.js Outdated Show resolved Hide resolved
@kaimoe kaimoe merged commit 4e980b2 into xivapi:master Jul 4, 2021
@laurayco laurayco deleted the use-async branch July 6, 2021 06:41
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.

2 participants