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

Fix type hints #2663

Merged
merged 7 commits into from Jul 1, 2022
Merged

Fix type hints #2663

merged 7 commits into from Jul 1, 2022

Conversation

daikikatsuragawa
Copy link
Contributor

@daikikatsuragawa daikikatsuragawa commented Jun 16, 2022

Related Issue or bug

Added and fixed about type hints. There is no associated Issue, but it enhances the readability and robustness of the code.

Type of change

  • Code style update (formatting, local variables)

How Has This Been Tested?

No changes were made that would change the behavior. If the CI test passes, you can be sure that there is no problem.

@daikikatsuragawa daikikatsuragawa changed the base branch from master to develop June 16, 2022 16:57
@daikikatsuragawa
Copy link
Contributor Author

The tests failed. I think there is a problem with this pre-PR commit. Does anyone know the details?

@ngupta23
Copy link
Collaborator

The tests failed. I think there is a problem with this pre-PR commit. Does anyone know the details?

Can you pull in the latest develop branch into your branch and check. This may have fixed it for now.

#2672

@daikikatsuragawa daikikatsuragawa changed the base branch from develop to master June 23, 2022 09:42
@daikikatsuragawa daikikatsuragawa changed the base branch from master to develop June 23, 2022 09:42
README.md Outdated
@@ -92,24 +98,6 @@ pip install pycaret[full]

<div align="left">

## ⚡ PyCaret Time Series Module (beta)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe instead of removing this completely, we should reword this and say that it is integrated with the main pycaret. We have a lot of posts that point to this section of the readme for Time series

@ngupta23
Copy link
Collaborator

ngupta23 commented Jun 25, 2022

Do we need datasets/electricity.csv? If this is a new dataset, we should also add it to https://github.com/pycaret/datasets instead.

Copy link
Collaborator

@ngupta23 ngupta23 left a comment

Choose a reason for hiding this comment

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

Most of this looks OK. I left a couple of minor comments that are unrelated to the actual PR about type hints. If you want, you can fix it here or leave them as is and submit a separate PR for those items to keep it clean (I will leave that decision up to you).

Thanks!

@daikikatsuragawa
Copy link
Contributor Author

Thanks for the comment. And my apologies.

There are unintended changes present, partly because the branch was created from the master. I will try to see if I can remove this (revert?).

@daikikatsuragawa
Copy link
Contributor Author

I have limited the PR diff to what was intended. I would appreciate your confirmation that this is correct.

https://github.com/pycaret/pycaret/pull/2663/files

@ngupta23 ngupta23 merged commit 4c0d232 into pycaret:develop Jul 1, 2022
@daikikatsuragawa daikikatsuragawa deleted the type-hints branch July 4, 2022 11:51
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