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

initial push #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

initial push #1

wants to merge 6 commits into from

Conversation

APEX101
Copy link
Owner

@APEX101 APEX101 commented Aug 9, 2021

Pull request

.idea/.gitignore Outdated
@@ -0,0 +1,3 @@
# Default ignored files

Choose a reason for hiding this comment

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

Please add all .idea files in gitignore

@@ -0,0 +1,50 @@
import pandas as pd

Choose a reason for hiding this comment

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

Use 'snake_case` for file names. Do note that mistakes will be pointed out only once. You will be assessed on how well you can incorporate suggestions into your projects.



#Query Analysis Calculation Class
class Calculations():

Choose a reason for hiding this comment

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

class Calculations:

Remove Brackets.

Remove comment. Give class a more expressive name so you don't need the comment.

#Query Analysis Calculation Class
class Calculations():

def __init__(self, dataframe,query):

Choose a reason for hiding this comment

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

    def __init__(self, dataframe, query):

Formatting (spacing) issues.

Use a formatter like black to handle formatting rather than doing it manually.


def perform_calculations_yearly(self):

if self.query=="high_temp":

Choose a reason for hiding this comment

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

Extra tabs. Formatting issue again.

Choose a reason for hiding this comment

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

Read PEP-8 for python style guide. Read it thoroughly. I won't be pointing out mistakes related to PEP-8 from now on.

def perform_calculations_yearly(self):

if self.query=="high_temp":
self.result=print("\nI read high tempereature query\n")

Choose a reason for hiding this comment

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

Why are we printing this stuff? Is it part of the required output?


def perform_plotting(self):
if self.charttype == "normal":
self.dataframe["PKT"] = pd.to_datetime(self.dataframe["PKT"])

Choose a reason for hiding this comment

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

Ahmad what's matplotlib doing here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Its plotting graphs for the report

@@ -0,0 +1,36 @@
import pandas as pd

Choose a reason for hiding this comment

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

Store all your 3rd party libraries in requirements.txt

venv/Project1.py Outdated
@@ -0,0 +1,53 @@
from ParsingClass import Parsing

Choose a reason for hiding this comment

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

What does Project1mean? What does it tell us about this project?

main.py Outdated
# Press the green button in the gutter to run the script.
if __name__ == '__main__':
print_hi('PyCharm')
print("This is my first pycharm program")

Choose a reason for hiding this comment

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

How do your run your project? Shouldn't the stuff in Project1 be called in here?

@imAliAzhar
Copy link

Also why are your project files in venv? venv only stores your 3rd party libraries.

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