Skip to content

added getnewargs() to str.rs#3811

Merged
fanninpm merged 4 commits into
RustPython:mainfrom
zer0who:hoo_rustpython
Jul 14, 2022
Merged

added getnewargs() to str.rs#3811
fanninpm merged 4 commits into
RustPython:mainfrom
zer0who:hoo_rustpython

Conversation

@zer0who

@zer0who zer0who commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

Added getnewargs() to str.rs and have tested on test_unicode.py(test_getnewargs())

@fanninpm

Copy link
Copy Markdown
Contributor

#3807 was recently merged, which changes how CI runs the test suites. Can you rebase your branch on a fresh copy of main? Once you've done that, feel free to run the following command:

cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v test_enum test_pickle test_pickletools test_unicode

(Of course, everything after the -v flag can be changed out with whatever tests you want. Or none at all, if you have plenty of time to spend.)

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great! you fixed many more than just a single test. Could you please remove expectedFailure decorators from other successful tests?

@zer0who

zer0who commented Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

@youknowone How can I know which are successful?

@zer0who

zer0who commented Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

@youknowone Oh, I received message on slack. I'll do that.

@fanninpm

Copy link
Copy Markdown
Contributor

How can I know which are successful?

Have you looked at the CI logs? They should give you a clue for where to start looking.

@fanninpm fanninpm 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 minor cleanup comments, and I think this should be good to go.

Comment thread Lib/test/test_pickle.py
Comment thread Lib/test/test_pickle.py
Comment thread Lib/test/test_pickletools.py
Comment thread Lib/test/test_pickletools.py

@fanninpm fanninpm 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.

I apologize if this wasn't clear, but I meant to say the entirety of the lines. Right now these lines are just redundant, and they can go away (along with the extra whitespace).

@zer0who

zer0who commented Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

You mean that delete the test 'functions' which I deleted expectedFailure annotation?

@fanninpm

Copy link
Copy Markdown
Contributor

You mean that delete the test 'functions' which I deleted expectedFailure annotation?

Any lines that now say

def test_function(self):
    super().test_function()

are just redundant at this point if they're not annotated with a @unittest.expectedFailure or a @unittest.skip decorator. They also make it harder to upgrade the test suite in the future.

If a test function has something else other than a call to super(), you can leave it be.

@zer0who

zer0who commented Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

Is this right?

@fanninpm fanninpm 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.

This is correct. Thank you!

@zer0who

zer0who commented Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

Thank you, too!!

@fanninpm

Copy link
Copy Markdown
Contributor

According to the CI, it seems you may have missed a test in test_pickle.py. Are you able to replicate what CI is showing?

@fanninpm fanninpm requested a review from youknowone July 3, 2022 19:32

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code changes look good! But maybe you didn't set your git email as your github email.
Do you want to change it before merge?

@youknowone

Copy link
Copy Markdown
Member

@zer0who please let me know if you don't care about email address. I don't mind to merge it as it is if you are ok.

@fanninpm

Copy link
Copy Markdown
Contributor

@zer0who please let me know if you don't care about email address. I don't mind to merge it as it is if you are ok.

Here is GitHub's documentation to set your commit email address.

In order to fix these commits, you can run the following commands:

git fetch upstream
git rebase --exec 'git commit --amend --reset-author' -i upstream/main

@zer0who

zer0who commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

@zer0who please let me know if you don't care about email address. I don't mind to merge it as it is if you are ok.

I don't know exatly that email you mentioned, but I did it as I understanded. Is it right?

@zer0who

zer0who commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

@zer0who please let me know if you don't care about email address. I don't mind to merge it as it is if you are ok.

Here is GitHub's documentation to set your commit email address.

In order to fix these commits, you can run the following commands:

git fetch upstream
git rebase --exec 'git commit --amend --reset-author' -i upstream/main

Thank you for your help. It was helpful for me. I'm not good at git... so I think it would be hard to explain to me about those things. But I did my best...!!:) Thank you again.

@youknowone

Copy link
Copy Markdown
Member

check git log in your branch. the email of your commit is not your actual email.
(I don't think your email is 62321708+zer0hoo@users.noreply.github.com)

@fanninpm

Copy link
Copy Markdown
Contributor

@zer0who please let me know if you don't care about email address. I don't mind to merge it as it is if you are ok.

Here is GitHub's documentation to set your commit email address.

In order to fix these commits, you can run the following commands:

git fetch upstream
git rebase --exec 'git commit --amend --reset-author' -i upstream/main

I forgot to add git push --force to that list.

@zer0who

zer0who commented Jul 14, 2022

Copy link
Copy Markdown
Contributor Author

Am I right??

@Snowapril

Copy link
Copy Markdown
Contributor

@zer0who Yeah It seems good now 😊

@fanninpm

Copy link
Copy Markdown
Contributor

Am I right??

Here's how you can tell:

  • On GitHub, you can look to see if your avatar is displayed next to each of your commits.
  • Locally, running git log will tell you who authored a given commit. There will normally be a line that says Author: Your Name <yourname@example.com>. If the email address in that commit matches one that GitHub recognizes (e.g. the one linked to your GitHub account), then GitHub will display your avatar next to each of your commits.

@zer0who

zer0who commented Jul 14, 2022

Copy link
Copy Markdown
Contributor Author

Am I right??

Here's how you can tell:

  • On GitHub, you can look to see if your avatar is displayed next to each of your commits.
  • Locally, running git log will tell you who authored a given commit. There will normally be a line that says Author: Your Name <yourname@example.com>. If the email address in that commit matches one that GitHub recognizes (e.g. the one linked to your GitHub account), then GitHub will display your avatar next to each of your commits.

Thank you so much :)

@fanninpm fanninpm merged commit 58015c1 into RustPython:main Jul 14, 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.

4 participants