AbsaOSS / spot

Aggregate and analyze Spark history, export to elasticsearch, visualize and monitor with Kibana.
Apache License 2.0
5 stars 0 forks source link

Initial commit #1

Closed DzMakatun closed 4 years ago

DzMakatun commented 4 years ago

Initial commit for Spot Code review and any suggestions are very welcome

Zejnilovic commented 4 years ago
  1. .iml and .DS_store should not be committed. That is IntelliJ IDEA specific
  2. Have you thought about utilizing editorconfig? Would autoformat a lot of stuff for you.

possible editorconfig:

# https://editorconfig.org/

root = true

[*]
indent_style = space
indent_size = 2
insert_final_newline = true
trim_trailing_whitespace = true
end_of_line = lf
charset = utf-8

[*.py]
max_line_length = 120
indent_size = 4

[docs/**.txt]
max_line_length = 80
DzMakatun commented 4 years ago
  1. .iml and .DS_store should not be committed. That is IntelliJ IDEA specific
  2. Have you thought about utilizing editorconfig? Would autoformat a lot of stuff for you.

possible editorconfig:

# https://editorconfig.org/

root = true

[*]
indent_style = space
indent_size = 2
insert_final_newline = true
trim_trailing_whitespace = true
end_of_line = lf
charset = utf-8

[*.py]
max_line_length = 120
indent_size = 4

[docs/**.txt]
max_line_length = 80

Many thanks, fixed gitignore and added editorconfig

DzMakatun commented 4 years ago

Why do you version images?

What is a better way to work with images in readme?

Zejnilovic commented 4 years ago

Why do you version images?

What is a better way to work with images in readme?

Ideally, you would use some form of ![alt text](http://url/to/img.png) where you have 2 options for hosting AFAIK:

DzMakatun commented 4 years ago

Why do you version images?

What is a better way to work with images in readme?

Ideally, you would use some form of ![alt text](http://url/to/img.png) where you have 2 options for hosting AFAIK:

  • we have bigusdatus and @hamc17 could help set something basic up
  • you could use GitHub

    • Upload image to a comment
    • This generates the URL and uploads image to GithHub
    • Use that URL in readme

spot_logo

architecture
DzMakatun commented 4 years ago

Overall looks good, minor nitpicky comments from me. Are you running PEP8, pylint or a similar tool on the Python files?

  • Yes, using PyCharm in Idea. I have reiterated the code and fixed all highlighted issues.
    It would be a good idea to pick a convention for quotations, e.g. double for interpolation, single for literal.
  • Agreed, thnx I would remove any deprecated functions/methods before merging.
  • Removed all relevant. There are few things which might be reused later. E.g. using dataset and schema metadata from Enceladus, I keep them for now. Where possible stick to f-strings for interpolation.
  • Sure, thnx. It would also be a good idea to include docstrings at some point to make clear what each function/method is doing to reviewers/contributors/users: https://www.python.org/dev/peps/pep-0257/
  • Created a new issue #7