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

#81 point 8. #83

Closed
wants to merge 3 commits into from
Closed

Conversation

ashrasmun
Copy link

Abstract

Fix for point 8. of Issue #81 aka changing from use of 'format' to f-strings.

Action Taken

Provided unit tests for 'format_result_rows' function and removed all the 'format(...)' uses on strings. Also added an assertion for 'index' and initialized the 'out' variable at the very beginning to ensure that all execution paths work properly.

Caveats

There are three things that bother me.

  1. I don't know how to test all the changes. When I invoke tests, they require a lot of dependencies. I'm not used to that to be honest.
  2. The change in PyPoE/cli/exporter/wiki/admin/unique.py is confusing for me. I removed the 'format' use and kept it consistent, but I don't understand why do we add a key named 'inventory_icon ' if we do not find the 'inventory_icon' key. Should it be that way or is it a bug?
  3. I introduced 'FakeArgsObject' in tests, but I think the proper thing would be to change 'format_result_rows' so that it accepts a string instead of an object. I believe there's no reason for this function to accept whole object if only one field is used.

FAO

After consultation with @pm5k, I'd like to ask @zao and @angelic-knight to take a quick look at the second issue from the Caveats section above.

ashrasmun added 3 commits May 21, 2022 13:31
Introduced in hope of not introducing regressions after small refactor
Only 'format_result_rows' is tested via unit tests
@ashrasmun
Copy link
Author

This is my first pull request ever so please tell me if I did something wrong so I can repair it :)

@pm5k
Copy link
Collaborator

pm5k commented Dec 12, 2022

OOS & OOD

@pm5k pm5k closed this Dec 12, 2022
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