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

Create a skip_validation parameter to yaml story reader #8334

Merged
merged 19 commits into from
Apr 19, 2021

Conversation

kalkbrennerei
Copy link
Contributor

@kalkbrennerei kalkbrennerei commented Mar 29, 2021

closes #8333

Proposed changes:

  • add a skip_validation flag to yaml_story_reader::read_from_file and yaml_story_reader::read_from_string

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)

@kalkbrennerei kalkbrennerei requested review from a team and wochinge and removed request for a team and wochinge March 29, 2021 16:20
@kalkbrennerei kalkbrennerei changed the title added the skip_validation flag to yaml story reader add the skip_validation flag to yaml story reader Mar 29, 2021
@kalkbrennerei kalkbrennerei changed the title add the skip_validation flag to yaml story reader Introduce a skip_validation flag to yaml story reader Mar 29, 2021
@kalkbrennerei kalkbrennerei changed the title Introduce a skip_validation flag to yaml story reader Create a skip_validation parameter to yaml story reader Mar 29, 2021
@kalkbrennerei kalkbrennerei requested a review from degiz March 30, 2021 13:33
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Thanks 👍

changelog/8333.bugfix.md Outdated Show resolved Hide resolved
@kalkbrennerei kalkbrennerei requested a review from degiz March 31, 2021 15:00
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Looks good! Can we add a unit tests though? 🤔

@kalkbrennerei kalkbrennerei requested a review from degiz April 8, 2021 13:30
Copy link
Contributor

@federicotdn federicotdn left a comment

Choose a reason for hiding this comment

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

Looks good! Do we also need to update the Markdown implementation of the reader? We should add the parameter there as well, even if we do not use it.

changelog/8333.bugfix.md Outdated Show resolved Hide resolved
@kalkbrennerei
Copy link
Contributor Author

Looks good! Do we also need to update the Markdown implementation of the reader? We should add the parameter there as well, even if we do not use it.

@federicotdn Since we'll drop markdown support, can we actually leave it out? Also I think that validation works differently in the markdown reader, so it might be tricky to add the skip_validation there afaik

@federicotdn
Copy link
Contributor

I actually was thinking of only adding the parameter (either named _ or _skip_validation), just so the method's signature is identical to it's parent class' (without any implementation).

@kalkbrennerei
Copy link
Contributor Author

ohhh sorry, I didn't read properly 🙈
I added the skip_validation parameter to the read_from_file method of the markdown reader without using it, so I think what you're asking is already there? Not sure if we're talking about the same thing though 😅

@federicotdn
Copy link
Contributor

Aaah I missed it! Yes exactly, that was what I was referring to 👍🏻 👍🏻

@kalkbrennerei kalkbrennerei enabled auto-merge April 19, 2021 13:42
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

let's goo 🚀

@kalkbrennerei kalkbrennerei merged commit 76c7836 into main Apr 19, 2021
@kalkbrennerei kalkbrennerei deleted the 8333-slow-domain-warnings-fix branch April 19, 2021 14:52
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.

Introduce a skip_validation parameter in yaml reader
4 participants