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

Change SafeYAML raise to a warn #9035

Closed
wants to merge 1 commit into from
Closed

Change SafeYAML raise to a warn #9035

wants to merge 1 commit into from

Conversation

aav7fl
Copy link

@aav7fl aav7fl commented Nov 12, 2020

Change SafeYAML from a raise to a warn to prevent breaking other dependencies that require it during the build steps before testing (e.g. Jekyll)

This change was added very recently in this PR: #9004

With the latest update to RuboCop (1.3.0), it raises an exception when SafeYAML exists.

I use Jekyll to build my website which depends on SafeYAML. In my testing phase I check for both linting errors and other issues with my site build. However, because RuboCop is in these steps, it finds my SafeYAML executable and fails with the raised exception.

My proposal is that we change this from a raised exception to a warning instead.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Change SafeYAML from a raise to a warn to prevent breaking other dependencies that require it during the build steps before testing (e.g. Jekyll)
@nonnenmacher
Copy link

+1 for this PR

Got same error and some Rake tasks don't load anymore, as we use a lot of legacy code that use the same kind of ERB -> YAML preprocessing as your ConfigLoader, it should not be the role of Rubocop to act as a RoboCop for other users (even if being right here).

We don't all have time to cope with this even if known.

A Warning is clean enough.

@marcandre
Copy link
Contributor

marcandre commented Nov 12, 2020

Sorry for the trouble.

Thanks for opening this issue.

I am opening an issue with Jekyll, and also with yaml that lacks a safe_load_file... Anyone else encounters other gems still dependent on SafeYAML?

I'm fine with not raising an error. Probably what I should have done is rescue an error from YAML.safe_load and only then raise that error if SafeYAML exists. @aav7fl are you willing to change your PR this way?

As long as only safe_yaml/load is loaded, it shouldn't interfere, but if there's a require 'safe_yaml', that overloads YAML.safe_load and I'd rather we don't have to support that.

@marcandre marcandre self-assigned this Nov 12, 2020
@aav7fl
Copy link
Author

aav7fl commented Nov 12, 2020

No worries! Thank you for taking the time to make improvements to the code and creating a ticket in the Jekyll project. :)

I don't appear to have any other gems that depend on SafeYAML.

I think agree with your proposal of raising the error from YAML.safe_load, but I'm tied up with other projects at the moment to dissect what's happening in the code.

Based on your comment, there's more going on here than just changing a raise/warn that I haven't fully digested

@marcandre I've given you collaborator access for my fork if you wanted to apply those changes to this PR. Or you can create a new PR and I will close 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.

None yet

3 participants