-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor the internals of analyze #194
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
Open
psykzz
wants to merge
9
commits into
WarcraftPriests:master
Choose a base branch
from
psykzz:feat/analyze-refactor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
661d29a
Refactor the internals of analyze
psykzz ca2adf2
Cleanup line length
psykzz 7d973ce
Merge remote-tracking branch 'upstream/master' into feat/analyze-refa…
psykzz 765e788
Adding common tests
psykzz eb74062
fix common test on linux
psykzz 78e31da
More generic paths
psykzz 2cd96ff
Fix linting
psykzz f7925ab
fix test_common linting
psykzz 1d55875
Merge branch 'master' into feat/analyze-refactor
psykzz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| 'internal configuration - exposes config.yml as a dict' | ||
| import yaml | ||
|
|
||
| config = {} | ||
| with open("config.yml", "r") as ymlfile: | ||
| config = yaml.load(ymlfile, Loader=yaml.FullLoader) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| 'tests for analyze module' | ||
|
|
||
| import os | ||
| import sys | ||
| from unittest.mock import call | ||
| sys.path.insert(0, os.path.abspath( # This should be the path back to the root directory. | ||
| os.path.join(os.path.dirname(__file__), '..', '..'))) | ||
|
|
||
| from internal.analyze import analyze # pylint: disable=wrong-import-position | ||
|
|
||
|
|
||
| def test_analyze(mocker): | ||
| 'test for the main analyze function' | ||
| spy_pandas = mocker.patch('pandas.read_csv', return_value={}) | ||
| spy_build_results = mocker.patch( | ||
| 'internal.analyze.build_results', return_value={}) | ||
| spy_build_md = mocker.patch( | ||
| 'internal.analyze.build_markdown', return_value=None) | ||
| spy_build_csv = mocker.patch( | ||
| 'internal.analyze.build_csv', return_value=None) | ||
| spy_build_json = mocker.patch( | ||
| 'internal.analyze.build_json', return_value=None) | ||
|
|
||
| # Not dungeon run | ||
| analyze("talent", "gear", False, "weights", "timestamp", "covenant") | ||
|
|
||
| spy_pandas.assert_called_once_with( | ||
| os.path.join('gear', 'output', 'talent', 'covenant', 'statweights.csv'), | ||
| usecols=['profile', 'actor', 'DD', 'DPS', | ||
| 'int', 'haste', 'crit', 'mastery', 'vers'] | ||
| ) | ||
| spy_build_results.assert_has_calls([ | ||
| call({}, 'weights', 'Composite', 'gear'), | ||
| call({}, 'weights', 'Single', 'gear') | ||
| ]) | ||
| spy_build_md.assert_has_calls([ | ||
| call('Composite', '_talent', {}, 'gear', 'weights', None, '_covenant'), | ||
| call('Single', '_talent', {}, 'gear', 'weights', None, '_covenant') | ||
| ]) | ||
| spy_build_csv.assert_has_calls([ | ||
| call('Composite', '_talent', {}, 'gear', 'weights', None, '_covenant'), | ||
| call('Single', '_talent', {}, 'gear', 'weights', None, '_covenant') | ||
| ]) | ||
| spy_build_json.assert_not_called() | ||
|
|
||
|
|
||
| def test_analyze_dungeon_run(mocker): | ||
| 'tests running analyze with the dungeon flag set true' | ||
| spy_pandas = mocker.patch('pandas.read_csv', return_value={}) | ||
| spy_build_results = mocker.patch( | ||
| 'internal.analyze.build_results', return_value={}) | ||
| spy_build_md = mocker.patch( | ||
| 'internal.analyze.build_markdown', return_value=None) | ||
| spy_build_csv = mocker.patch( | ||
| 'internal.analyze.build_csv', return_value=None) | ||
| spy_build_json = mocker.patch( | ||
| 'internal.analyze.build_json', return_value=None) | ||
|
|
||
| # Dungeon run | ||
| analyze("talent", "gear", True, "weights", "timestamp", "covenant") | ||
|
|
||
| spy_pandas.assert_called_once_with( | ||
| os.path.join('gear', 'output', 'talent', 'covenant', 'statweights.csv'), | ||
| usecols=['profile', 'actor', 'DD', 'DPS', | ||
| 'int', 'haste', 'crit', 'mastery', 'vers'] | ||
| ) | ||
| spy_build_results.assert_called_once_with( | ||
| {}, 'weights', 'Dungeons', 'gear') | ||
| spy_build_md.assert_called_once_with( | ||
| 'Dungeons', '_talent', {}, 'gear', 'weights', None, '_covenant' | ||
| ) | ||
| spy_build_csv.assert_called_once_with( | ||
| 'Dungeons', '_talent', {}, 'gear', 'weights', None, '_covenant' | ||
| ) | ||
| spy_build_json.assert_not_called() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| 'tests for config module' | ||
|
|
||
| import os | ||
| import sys | ||
| import yaml | ||
|
|
||
| sys.path.insert(0, os.path.abspath( # This should be the path back to the root directory. | ||
| os.path.join(os.path.dirname(__file__), '..', '..'))) | ||
|
|
||
| from internal.config import config # pylint: disable=wrong-import-position | ||
|
|
||
| def test_config(): | ||
| 'test loading the config' | ||
| original_config = None | ||
| with open("config.yml", "r") as ymlfile: | ||
| original_config = yaml.load(ymlfile, Loader=yaml.FullLoader) | ||
| assert original_config is not None, 'unable to load config' | ||
|
|
||
| # yeah this test is pretty eh, but not sure what to do here. | ||
| assert config == original_config |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| 'Common helpers for writers' | ||
| import os | ||
|
|
||
| from internal.config import config | ||
| from internal.weights import find_weights | ||
| from internal.spell_ids import find_ids | ||
|
|
||
|
|
||
| def generate_report_name(sim_type, talent=None, covenant=None): | ||
| """create report name based on talents and covenant""" | ||
| talents = f" - {talent.strip('_')}" if talent else "" | ||
| covenant = f" - {covenant.strip('_')}" if covenant else "" | ||
| return f"{sim_type}{talents}{covenant}" | ||
|
|
||
|
|
||
| def assure_path_exists(path): | ||
| """Make sure the path exists and contains a folder""" | ||
| dir_name = os.path.dirname(path) | ||
| if not os.path.exists(dir_name): | ||
| os.makedirs(dir_name) | ||
|
|
||
|
|
||
| def build_output_string(base_path, sim_type, talent, covenant, file_ext): | ||
| """creates output string for the results file""" | ||
| output_dir = os.path.join(base_path, "results") | ||
| assure_path_exists(output_dir) | ||
| return os.path.join(output_dir, f"Results_{sim_type}{talent}{covenant}.{file_ext}") | ||
|
|
||
|
|
||
| def lookup_id(name, directory): | ||
| """lookup the spell or item id of an item name""" | ||
| lookup_type = config["sims"][directory]["lookupType"] | ||
| if lookup_type == "spell": | ||
| return lookup_spell_id(name, directory) | ||
| if lookup_type == "item": | ||
| return lookup_item_id(name, directory) | ||
| if lookup_type == "none": | ||
| return None | ||
| print(f"Could not find id for {name}") | ||
| return None | ||
|
|
||
|
|
||
| def lookup_spell_id(spell_name, directory): | ||
| """lookup a spell name from the ids dict""" | ||
| ids = find_ids(directory) | ||
| if ids: | ||
| return ids.get(spell_name) | ||
| print(f"Could not find spell id for {spell_name}") | ||
| return None | ||
|
|
||
|
|
||
| def lookup_item_id(item_name, directory): | ||
| """ | ||
| get the list of sim files from config | ||
| loop over them and search for the item name line by line | ||
| """ | ||
| for sim_file in config["sims"][directory]["files"]: | ||
| with open(sim_file, 'r') as file: | ||
| for line in file: | ||
| if item_name in line: | ||
| # find ,id= -> take 2nd half -> | ||
| # find , -> take 1st half | ||
| return int(line.split(',id=')[1].split(',')[0]) | ||
| return None | ||
|
|
||
|
|
||
| def convert_increase_to_double(increase): | ||
| """convert string increase to double""" | ||
| increase = increase.strip('%') | ||
| increase = round(float(increase), 4) | ||
| if increase: | ||
| increase = round(increase / 100, 4) | ||
| return increase | ||
|
|
||
|
|
||
| def get_change(current, previous): | ||
| """gets the percent change between two numbers""" | ||
| negative = 0 | ||
| if current < previous: | ||
| negative = True | ||
| try: | ||
| value = (abs(current - previous) / previous) * 100.0 | ||
| value = float('%.2f' % value) | ||
| if value >= 0.01 and negative: | ||
| value = value * -1 | ||
| return value | ||
| except ZeroDivisionError: | ||
| return 0 | ||
|
|
||
|
|
||
| def find_weight(sim_type, profile_name): | ||
| """looks up the weight based on the sim type""" | ||
| weight_type = "" | ||
| if sim_type == "Composite": | ||
| weight_type = "compositeWeights" | ||
| elif sim_type == "Single": | ||
| weight_type = "singleTargetWeights" | ||
| elif sim_type == "Dungeons": | ||
| # Dungeon sim is just 1 sim, so we return 1 here | ||
| return 1 | ||
| weight = find_weights(config[weight_type]).get(profile_name) | ||
| if not weight: | ||
| return 0 | ||
| return weight |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wont this fail? Since
args.diristalents/but we need to lookup based ontalentsUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No since we now normalise the path, so a user could do
./talentsor./talents/it would be normalised totalentsSee https://github.com/WarcraftPriests/sl-shadow-priest/pull/194/files#diff-ec7f77098f7800c209790b5e76053a876d0bb993248972a80a8941a573ba9323R213