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

Add __repr__ to Token - Related to Issue #7250, PR #7839 #9543

Closed
wants to merge 481 commits into from

Conversation

ErickGiffoni
Copy link
Contributor

@ErickGiffoni ErickGiffoni commented Sep 6, 2021

Issue #7250 was previously solved via PR #7839 but there was some trouble during the checks.
The conversation in the PR 7839 ended, so I continued the work.

Proposed changes:

  • Fixes a typo warned by the typo CI

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@ErickGiffoni ErickGiffoni requested a review from a team as a code owner September 6, 2021 13:35
@ErickGiffoni ErickGiffoni requested review from ancalita and removed request for a team September 6, 2021 13:35
@ancalita
Copy link
Member

ancalita commented Sep 6, 2021

Hi @ErickGiffoni Thank you for looking into this 🚀
Since the branch issue-7250 is quite outdated, I'd recommend starting from scratch and have your new PR target main branch to close this issue. Once you've done that, this PR should be closed.

@ErickGiffoni
Copy link
Contributor Author

ErickGiffoni commented Sep 6, 2021

Hi @ErickGiffoni Thank you for looking into this 🚀
Since the branch issue-7250 is quite outdated, I'd recommend starting from scratch and have your new PR target main branch to close this issue. Once you've done that, this PR should be closed.

Ok, no problem !

Would it be ok if I just change this PR's base branch to "main", since I've merged both branches ?
I see that this PR has got filled with too many files to review lol but that's probably because of the merging
with main and the target being issue-7250 as you pointed out.

@ancalita
Copy link
Member

ancalita commented Sep 7, 2021

Would it be ok if I just change this PR's base branch to "main"

@ErickGiffoni Sure, as long as your changes/ commits become visible for review.

@ErickGiffoni ErickGiffoni changed the base branch from issue-7250 to main September 7, 2021 18:45
@ErickGiffoni ErickGiffoni requested review from a team and aeshky and removed request for a team September 7, 2021 18:45
@ErickGiffoni
Copy link
Contributor Author

@ancalita done. Thanks for reviewing this ! I'll be awaiting for further feedback.

@aeshky
Copy link
Contributor

aeshky commented Sep 8, 2021

Thanks for your contribution, @ErickGiffoni.
Can you please add a change log file (the instructions here). Also, can you target the 2.8.x branch instead of main?

@ErickGiffoni
Copy link
Contributor Author

Thanks for your contribution, @ErickGiffoni.
Can you please add a change log file (the instructions here). Also, can you target the 2.8.x branch instead of main?

Yeah, sure, as you wish ! I’ll do it as soon as possible, thanks.

@ErickGiffoni ErickGiffoni changed the base branch from main to 2.8.x September 8, 2021 21:07
@ErickGiffoni
Copy link
Contributor Author

ErickGiffoni commented Sep 8, 2021

Thanks for your contribution, @ErickGiffoni.
Can you please add a change log file (the instructions here). Also, can you target the 2.8.x branch instead of main?

@aeshky Done! I did what you asked me to do. First the change log file. But when changing the target branch, lots of new files were changed. Is it ok or should I do differently ?
Also, now there are 2 conflicts, what should we do ?

@aeshky
Copy link
Contributor

aeshky commented Sep 9, 2021

Thanks for making the changes @ErickGiffoni. I should have been clearer: can you rebase your branch on 2.8.x and then target it for the merge? That way your change gets released in the next micro release instead of waiting for the next major release. So the only difference should be your changes.

dependabot bot and others added 13 commits September 9, 2021 10:12
Bumps [styfle/cancel-workflow-action](https://github.com/styfle/cancel-workflow-action) from 0.6.0 to 0.9.1.
- [Release notes](https://github.com/styfle/cancel-workflow-action/releases)
- [Commits](styfle/cancel-workflow-action@0.6.0...a40b884)

---
updated-dependencies:
- dependency-name: styfle/cancel-workflow-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Implement core graph parts and runner.
 CachedComponent

Co-authored-by: Tobias Wochinger <[email protected]>
* Add ability to point to a certificate in endpoints.yml (RasaHQ#9118)

* Add cert file functionality to Endpoint

* Apply suggestions from code review

types

Co-authored-by: Tobias Wochinger <[email protected]>

* update changelog

Co-authored-by: Tobias Wochinger <[email protected]>

* Fix epoch override for TEDPolicy (RasaHQ#9182)

* bump rasa-sdk dep and min compat version

* fix bug add tests

* add changelog

* change test cases

* remove test cases

* prepared release of version 2.8.1 (RasaHQ#9183)

* update rasa-sdk

* prepared release of version 2.8.1

* update lock

* Make UnexpecTEDIntentPolicy compatible with E2E data (RasaHQ#9203)

* bump rasa-sdk dep and min compat version

* add test and code

* add changelog and refactor method

* add test

* more test cases

* use applied_events

* 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (RasaHQ#9237)

* I removed the experimental feature designation from the docs.

* I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example.

* I removed the unused import: rasa.shared.utils.common

* Adding change log files for issues 8791 and 8024.

* Rewording change logs to make it clear that the behaviour of the features remains unchanged.

* prepared release of version 2.8.2 (RasaHQ#9262)

* prepared release of version 2.8.2

* Updated the date of release.

* install yarn dependency in cloned repository, in docs publication workflow

* silence yarn warning

* Fix typo in push_docs_to_branch.sh

* Generated a new poetry lock file

Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Daksh Varshneya <[email protected]>
Co-authored-by: m-vdb <[email protected]>
* update-toml

* added-changelog

* poetry-lock

* update-lock-file-again
Bumps [pydoc-markdown](https://github.com/NiklasRosenstein/pydoc-markdown) from 3.13.0 to 4.1.6.
- [Release notes](https://github.com/NiklasRosenstein/pydoc-markdown/releases)
- [Commits](NiklasRosenstein/pydoc-markdown@v3.13.0...v4.1.6)

---
updated-dependencies:
- dependency-name: pydoc-markdown
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
wochinge and others added 21 commits September 9, 2021 10:39
Bumps [regex](https://bitbucket.org/mrabarnett/mrab-regex) from 2021.7.6 to 2021.8.28.
- [Commits](https://bitbucket.org/mrabarnett/mrab-regex/commits)

---
updated-dependencies:
- dependency-name: regex
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [moto](https://github.com/spulec/moto) from 2.0.11 to 2.2.6.
- [Release notes](https://github.com/spulec/moto/releases)
- [Changelog](https://github.com/spulec/moto/blob/master/CHANGELOG.md)
- [Commits](getmoto/moto@2.0.11...2.2.6)

---
updated-dependencies:
- dependency-name: moto
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
In light of the fact the predictions list can only contain 0 or 1 messages, we don't need a test that tests for 2 messages. Also, drop setting metadata in the Message as that is now carried over through the original UserMessage which is not parameterised
@ErickGiffoni
Copy link
Contributor Author

ErickGiffoni commented Sep 9, 2021

Thanks for making the changes @ErickGiffoni. I should have been clearer: can you rebase your branch on 2.8.x and then target it for the merge? That way your change gets released in the next micro release instead of waiting for the next major release. So the only difference should be your changes.

@aeshky Ok, done it ! I never solved soo many conflicts in my life ( lol ).

I rebased it like this: git pull --rebase upstream 2.8.x, where "upstream" is set to here, RasaHQ/rasa.

Obs.: from the beginning my current branch was always issue-7250

Then, after solving lots of conflicts, I tried to git push origin issue-7250 (my origin is set to my repository), but it says my current branch is behind the remote one. Maybe makes sense, since initially my branch issue-7250 was created from branch main.

But now it is rebased, so it's like it was created from 2.8.x which is probably behind main.
My point is that I am so confused right now that I need help, lol. I'm sorry.

What should I do ?

Should I just git push -f origin issue-7250 ?

Found this reference on Stack Overflow, it is exactly my problem: https://stackoverflow.com/questions/8939977/git-push-rejected-after-feature-branch-rebase .

@aeshky
Copy link
Contributor

aeshky commented Sep 14, 2021

Hey @ErickGiffoni. Sorry you had to resolve so many conflicts! Your original submission only had changes to a couple of files, but it looks like rebasing your branch on 2.8.x created too many conflicts.

I think the easiest way to resolve this is to create a new branch from 2.8.x, then move your commits to it (using git cherry-pick). You can then create a new pull request and tag me in it. How does that sound?

@ErickGiffoni
Copy link
Contributor Author

Hey, @aeshky, ok that sounds good for me, I'm on it. Looking forward to have my contribution accepted !

@ErickGiffoni
Copy link
Contributor Author

New PR created: #9631

Closing this one !

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.