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

Enduring Luminescence #2361

Closed
wants to merge 4 commits into from

Conversation

rp4rk
Copy link
Collaborator

@rp4rk rp4rk commented Oct 2, 2018

Implements the Enduring Luminescence trait for Discipline Priests.

Copy link
Contributor

@joshinat0r joshinat0r left a comment

Choose a reason for hiding this comment

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

Just some nitpicking on the formatting, the code itself looks good.


handlePWRBonus(event) {
const spellId = event.ability.guid;
if (spellId !== SPELLS.POWER_WORD_RADIANCE.id) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

if () {
    return;
}

just to keep it consistent across the code-base. I personally prefer it this way aswell, but 🤷‍♂️

super(...args);

this.ranks =
this.selectedCombatant.traitRanks(SPELLS.ENDURING_LUMINESCENCE.id) || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Zero had a few weeks ago an argument about max-line length stuff and making too much use of the enter-key.
This should be one line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah sure, thanks for the feedback on formatting. Personally I use prettier with an 80 char max length for diffing side by side, but it's not a huge deal.

this.selectedCombatant.traitRanks(SPELLS.ENDURING_LUMINESCENCE.id) || [];
this.bonusHealing =
this.ranks
.map(rank =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I think we usually format it this way.

this.bonusHealing = this.ranks
                     .map(rank => calculateAzeriteEffects(SPELLS.ENDURING_LUMINESCENCE.id, rank))
                     .reduce((total, bonus) => total + bonus, 0) * HEALING_MULTIPLIER;

const target = this.atonement.currentAtonementTargets.find(
id => id.target === event.targetID
);
if (!target) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Return

id => id.target === event.targetID
);
if (!target) return;
if (!target.isEnduringEmpowered) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Return

event.timestamp <
target.atonementExpirationTimestamp - EnduringLuminescence.bonusDuration
)
{return;}
Copy link
Contributor

Choose a reason for hiding this comment

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

if (
      event.timestamp <
      target.atonementExpirationTimestamp - EnduringLuminescence.bonusDuration
    ) {
    return;
}

category={STATISTIC_CATEGORY.AZERITE_POWERS}
position={STATISTIC_ORDER.OPTIONAL()}
icon={
(
Copy link
Contributor

Choose a reason for hiding this comment

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

The (-bracket isn't necessary and the spellIcon should be in the same line as the icon-prop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This breaks our linting unfortunately, I'll meet at the middle.

@joshinat0r joshinat0r closed this Oct 8, 2018
@joshinat0r joshinat0r reopened this Oct 8, 2018
@TravisBuddy
Copy link

Travis tests have failed

Hey @rp4rk,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

Node.js: 10.4

View build log

echo "> The server has already been built so we no longer need the devDependencies (reduces the final bundle size)"
> The server has already been built so we no longer need the devDependencies (reduces the final bundle size)
if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then echo "> Disabling sourcemap generation to save build time, this is a PR so won't be deployed anyway."; export GENERATE_SOURCEMAP=false; fi
> Disabling sourcemap generation to save build time, this is a PR so won't be deployed anyway.
npm run build
> wowanalyzer@ build /home/travis/build/WoWAnalyzer/WoWAnalyzer
> react-scripts build

Creating an optimized production build...
Failed to compile.

./src/parser/priest/discipline/modules/azeritetraits/EnduringLuminescence.js
Cannot find module: 'parser/core/modules/StatTracker'. Make sure this package is installed.

You can install this package by running: npm install parser/core/modules/StatTracker.


npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! wowanalyzer@ build: `react-scripts build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the wowanalyzer@ build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2018-10-08T08_54_01_785Z-debug.log
TravisBuddy Request Identifier: c85eb730-cad7-11e8-8b3f-99c2825e6a65

@joshinat0r
Copy link
Contributor

Travis failed because the StatTracker is now located in src/parser/shared/modules/StatTracker

return;
}

let eventHealing = this.bonusHealing;
Copy link
Contributor

Choose a reason for hiding this comment

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

In general traits are affected by secondary stats and multipliers (except primary stat).

Copy link
Contributor

@joshinat0r joshinat0r left a comment

Choose a reason for hiding this comment

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

StatTracker location

@TravisBuddy
Copy link

Travis tests have failed

Hey @rp4rk,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

docker build -f .docker/production/Dockerfile --tag wowanalyzer .
Step 1/24 : FROM node:10.12-alpine as app
10.12-alpine: Pulling from library/node



Digest: sha256:1e3e3e7ffc965511c5d4f4e90ec5d9cabee95b5b1fbcd49eb6a2289f425cf183
Status: Downloaded newer image for node:10.12-alpine
 ---> 7ca2f9cb5536
Step 2/24 : WORKDIR /usr/src/app/
 ---> 3f6d1ecd5d57
Removing intermediate container 16fe7f4dfbdd
Step 3/24 : COPY package.json package-lock.json /usr/src/app/
 ---> 1e5efc284185
Step 4/24 : RUN npm install
 ---> Running in 4f4d3ec5e6f0
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.4 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 1898 packages from 878 contributors and audited 62015 packages in 33.903s
found 0 vulnerabilities

 ---> 65834dd7e610
Removing intermediate container 4f4d3ec5e6f0
Step 5/24 : COPY . /usr/src/app/
 ---> 5ee7c724382e
Step 6/24 : RUN npm run build
 ---> Running in f8e1affd1aaf

> wowanalyzer@ build /usr/src/app
> react-scripts build

Creating an optimized production build...
Failed to compile.

./src/parser/priest/discipline/modules/azeritetraits/EnduringLuminescence.js
Cannot find module: 'parser/core/modules/StatTracker'. Make sure this package is installed.

You can install this package by running: npm install parser/core/modules/StatTracker.


npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! wowanalyzer@ build: `react-scripts build`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the wowanalyzer@ build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2018-11-12T22_52_48_224Z-debug.log
The command '/bin/sh -c npm run build' returned a non-zero code: 1
TravisBuddy Request Identifier: addf0b00-e6cd-11e8-87c2-ebfa2f2e9d99

@Dambroda
Copy link
Contributor

No update in a month, and I see some other things that will need changing. Just re-open the PR if you are active and I will put in my review :)

@Dambroda Dambroda closed this Nov 19, 2018
@rp4rk rp4rk deleted the feature/enduring-luminescence branch October 18, 2020 03:37
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.

6 participants