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

Added type annotations to streams.py and message.py #231

Merged
merged 13 commits into from Jun 29, 2022

Conversation

HivaMohammadzadeh1
Copy link
Contributor

@HivaMohammadzadeh1 HivaMohammadzadeh1 commented Jun 10, 2022

I added the type annotations to Streams.py and message.py and tested it from examples/simple_pipeline.py by adding and running the following code:
def f() -> Stream[int]:
pass
Still trying to figure out if I can add type annotations to split_by_type

@sukritkalra
Copy link
Collaborator

Hi! Thanks for the PR, could you please remove your environment from the commit so we could look at the changed files? Maybe try the following command: git rm --cached -r my-env

@HivaMohammadzadeh1
Copy link
Contributor Author

HivaMohammadzadeh1 commented Jun 10, 2022 via email

@sukritkalra
Copy link
Collaborator

No need for that, just add a new commit, and we can squash and merge the commits once the PR is ready.

@HivaMohammadzadeh1
Copy link
Contributor Author

I just did. See if it's fixed. Sorry for the inconvenience.

Copy link
Collaborator

@sukritkalra sukritkalra left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please take a look at my comments:

python/erdos/message.py Outdated Show resolved Hide resolved
python/erdos/message.py Outdated Show resolved Hide resolved
python/erdos/message.py Outdated Show resolved Hide resolved
python/examples/simple_pipeline.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
@HivaMohammadzadeh1
Copy link
Contributor Author

Thank you for your comments. I have considered all the comments and changed my code accordingly. Please let me know if there's anything that I still have to fix.
Also, for the function "split_by_type", I attempted to add type annotations to the code, but I'm not sure if it is fixed.

Copy link
Collaborator

@sukritkalra sukritkalra left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Please run the tools as I've suggested, and we can wait for @pschafhalter to approve the changes too.

python/erdos/message.py Outdated Show resolved Hide resolved
super(WatermarkMessage, self).__init__(timestamp, None)

def __str__(self):
def __str__(self) :
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why the CI is not running. But, could you please run the following from your python directory?

black .
isort --profile black .
flake8 --inline-quotes=double ./

and commit any changes that these tools make?

Copy link
Member

Choose a reason for hiding this comment

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

We need to manually approve and run the CI because Hiva is a first-time committer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! @HivaMohammadzadeh1 the CI should now show the errors in formatting, please take a look and address the issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After running flake8 --inline-quotes=double ./
I get this error. Is this normal?

usage: flake8 [options] file file ...
flake8: error: unrecognized arguments: --inline-quotes=double

It does not show me any errors in formatting, so I think that error might not be normal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably have to install the inline-quotes extension for flake8. Try doing this on your shell (within the virtual environment):

pip install flake8-quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect that was the problem. I ran and resolved the issues and committed.

python/erdos/streams.py Outdated Show resolved Hide resolved
python/examples/simple_pipeline.py Outdated Show resolved Hide resolved
Copy link
Member

@pschafhalter pschafhalter left a comment

Choose a reason for hiding this comment

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

A few small nits. Happy to merge once they're addressed and the formatting checks pass!

python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
python/erdos/streams.py Outdated Show resolved Hide resolved
@HivaMohammadzadeh1
Copy link
Contributor Author

I just did the changes. The commands run on my end. I wonder why they don't on here. Thank you for being patient with me and helping me

@sukritkalra
Copy link
Collaborator

Hi! I had to manually approve the workflow. It seems like your formatter made some changes in python/examples/linq.py and python/erdos/context.py that are not correct. Could you run black . again locally, fix the files and commit the changes please?

@HivaMohammadzadeh1
Copy link
Contributor Author

Just committed again :)

@HivaMohammadzadeh1
Copy link
Contributor Author

Yes, I figured it was doing that, so I didn't run black . as that was what was formatting it.

@sukritkalra
Copy link
Collaborator

sukritkalra commented Jun 29, 2022

Looks like its still failing, are these changes being made by black? Are you running a special black profile?

I just ran a new installation of black on your fork of the repository, and it seems to be in sync with the CI.

@HivaMohammadzadeh1
Copy link
Contributor Author

I just manually went and fixed it. Can you check if it works now? I didn't run black .

@sukritkalra
Copy link
Collaborator

Nope, still not working. I wonder why your black invocation is not fixing this. If you type black . on your command line, it should automatically reformat the context.py to what we had before.

@HivaMohammadzadeh1
Copy link
Contributor Author

I just ran black . and committed to see if it's fixed

@sukritkalra
Copy link
Collaborator

Nope, just for reference, my black version is 22.6.0 and it seems to fix the errors being raised by the CI.

@HivaMohammadzadeh1
Copy link
Contributor Author

I don't know why it's reformatting it wrong.

Copy link
Collaborator

@sukritkalra sukritkalra left a comment

Choose a reason for hiding this comment

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

Fixed the formatting issues for you, but there's issues in the pythondoc that need to be fixed.

python/erdos/streams.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@sukritkalra sukritkalra left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

@sukritkalra sukritkalra merged commit 3d05c60 into erdos-project:master Jun 29, 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.

None yet

3 participants