Skip to content

making get_brawlers (issue #60)#61

Closed
0dminnimda wants to merge 11 commits into
SharpBit:masterfrom
0dminnimda:issue_60_making_get_brawlers
Closed

making get_brawlers (issue #60)#61
0dminnimda wants to merge 11 commits into
SharpBit:masterfrom
0dminnimda:issue_60_making_get_brawlers

Conversation

@0dminnimda

Copy link
Copy Markdown
Contributor

Changes Description

  1. all py files were refactored in pep8 style
    (except for 97 lines in utils.py)
  2. splitting BaseBox into BaseBox and BaseBoxList for convenience
    (with the corresponding estates of both the models themselves and the models inheriting these models)
  3. added:
  • get_brawlers function
  • model Brawlers
  • BRAWLERS_URL in API
  • nothing function to replace lambda expressions in typecasted
  1. changed async.py and sync.py (now they are completely identical, in terms of meaning)
  2. adding tests to files from tests /
  3. tox for py38 now works
  4. adding documentation to CHANGELOG.md, api.rst, index.rst
    7.5) add some.py to gitignore

Checklist

  • [yes] Docstrings added (NumpyDoc)
  • [added] If necessary, add to the documentation files
  • [yes] Tox checked

major changes:
1) all py files were refactored in pep8 style
2) splitting BaseBox into BaseBox and BaseBoxList for convenience
(with the corresponding estates of both the models themselves and the models inheriting these models)
3) added:
- get_brawlers function
- model Brawlers
- BRAWLERS_URL in API
- nothing function to replace lambda expressions in typecasted
4) changed async.py and sync.py (now they are completely identical, in terms of meaning)
major changes:
1) all py files were refactored in pep8 style
(except for 97 lines in utils.py)
2) splitting BaseBox into BaseBox and BaseBoxList for convenience
(with the corresponding estates of both the models themselves and the models inheriting these models)
3) added:
- get_brawlers function
- model Brawlers
- BRAWLERS_URL in API
- nothing function to replace lambda expressions in typecasted
4) changed async.py and sync.py (now they are completely identical, in terms of meaning)

.. add some of my files to gitignore
@0dminnimda 0dminnimda changed the title making get brawlers (issue #60) making get_brawlers (issue #60) Jun 27, 2020
@SharpBit

Copy link
Copy Markdown
Owner

Please revert everything and readd only the tests for get_brawler, the models/functions for get_brawler and the fix for tox for 3.8

  • Please don't mess with code formatting, I purposely have the character limit as 120 chars since pep8's 80 character limit per line is just plain stupid and inconvenient
  • You've added random f strings on docstrings where they're not needed and left in some Python-3.5-incompatible code (some fstrings). Again, please revert and only add the code needed for get_brawler and the changes to tox that lets you run it for Python 3.8

@0dminnimda

Copy link
Copy Markdown
Contributor Author

I corrected everything, but you should know that pep8 standards did not come from the air!

@fourjr fourjr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

some of my feedback regarding code clarity

Comment thread .gitignore Outdated
@@ -1,3 +1,5 @@
some.py

@fourjr fourjr Jun 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra files should not be added to the .gitignore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nothing bad here, I just need to ignore some files. it does not spoil, you can remove this change

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If you need another file, move it into another directory - do not add unnecessary stuff to the project's .gitignore file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok

Comment thread CHANGELOG.md
### Changed
- splitting `BaseBox` into `BaseBox` and `BaseBoxList` for convenience
### Fixed
- tox for py38 works (not completely sure)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tox should be tested first...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not often create tox files (so I'm not sure), but it works fine for me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

here too that to the bottom post

Comment thread brawlstats/utils.py

# do nothing
def nothing(value):
return value

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i dont think its necessary to create a function here, the lambda works just fine and is cleaner

Comment thread brawlstats/utils.py
converter = param.annotation
if converter is inspect._empty:
converter = lambda a: a # do nothing
converter = nothing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i dont think its necessary to create a function here, the lambda works just fine and is cleaner

@0dminnimda 0dminnimda Jun 27, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lambda expression is created every time, and the function is defined, I think that it is worth avoiding lambda expressions, there is no problem to remove it here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lambda expression can result in unexpected performance issues

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so I would recommend you leave this part alone

@SharpBit

Copy link
Copy Markdown
Owner

Transferred to PR #62

@SharpBit SharpBit closed this Jun 27, 2020
@0dminnimda
0dminnimda deleted the issue_60_making_get_brawlers branch July 22, 2020 14:56
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.

3 participants