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

Remove ruby-enum dependency #140

Merged
merged 2 commits into from
Jun 20, 2021
Merged

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Jun 13, 2021

Fixes #135

@ojab ojab force-pushed the drop_ruby_enum branch from 47c9b9e to 93adf29 Compare June 13, 2021 22:21
when Array
option = [nil] if option.empty?
raise TypeError if option.none?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno what error message should I put here, but existing option ':' does not exist for CommonMarker::Config::Render looks wrong.

Overall I think that it could be just OPTS.fetch(type).fetch_values(*Array(option)).inject(0, :|) and that's it.
Right now we're overly strict in accepted types (why Set.new([:DEFAULT]) cannot be passed?) and IMHO it's expected that [] will be counted as a use defaults.

Copy link
Owner

Choose a reason for hiding this comment

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

Good points. Regarding being "overly strict"—I have a tendency to lean towards paranoia when interactions with C occur, hence the small whitelist of accepted types.

@ojab
Copy link
Contributor Author

ojab commented Jun 13, 2021

Sending to review due to First-time contributors need a maintainer to approve running workflows.. bundler exec rake passes locally.

@ojab ojab marked this pull request as ready for review June 13, 2021 22:23
@ojab
Copy link
Contributor Author

ojab commented Jun 14, 2021

Ugh, CI is failing due to

commonmarker.gemspec:24:29: C: Gemspec/RequiredRubyVersion: required_ruby_version (2.4, declared in commonmarker.gemspec) and TargetRubyVersion (2.5, which may be specified in .rubocop.yml) should be equal.
  s.required_ruby_version = ['>= 2.4.10', '< 4.0']
                            ^^^^^^^^^^^^^^^^^^^^^^

(rubocop dropped 2.4 support a while ago, so unfixable)

lib/commonmarker/node/inspect.rb:30:13: C: [Correctable] Style/RedundantBegin: Redundant begin block detected.
            begin
            ^^^^^

which is required and not redundant on 2.4.

Also specs are failing for 2.4 locally (KeyError does not have #key).
Since CI on 2.4 is not running — is it really supported or constraint in gemfile can be bumped to 2.5?

@gjtorikian
Copy link
Owner

Since CI on 2.4 is not running — is it really supported or constraint in gemfile can be bumped to 2.5?

Let's actually bring the constraint to 2.6, and remove 2.5 from CI. 2.5 is EOL and I don't want to support EOL Rubies. Thanks for bringing this up!

@ojab ojab force-pushed the drop_ruby_enum branch 3 times, most recently from 2a2d1c4 to 414d228 Compare June 18, 2021 19:32
@ojab
Copy link
Contributor Author

ojab commented Jun 18, 2021

Done, first commit bumps ruby to 2.6, second actually removing ruby-enum. And again First-time contributors need a maintainer to approve running workflows..

@ojab ojab force-pushed the drop_ruby_enum branch from 414d228 to ea0f742 Compare June 20, 2021 11:33
@gjtorikian
Copy link
Owner

Thank you for your contribution here, I really appreciate it.

@gjtorikian gjtorikian merged commit 3684416 into gjtorikian:main Jun 20, 2021
fauno added a commit to fauno/jekyll-commonmark that referenced this pull request Jun 22, 2021
CommonMarker 0.22 removed the Parser class and breaks jekyll-commonmark

gjtorikian/commonmarker#140
fauno added a commit to fauno/jekyll-commonmark that referenced this pull request Jun 22, 2021
CommonMarker 0.22 removed the Parser class and breaks jekyll-commonmark

gjtorikian/commonmarker#140
fauno added a commit to fauno/jekyll-commonmark that referenced this pull request Jun 22, 2021
CommonMarker 0.22 removed the Parser class and breaks jekyll-commonmark

gjtorikian/commonmarker#140
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.

Open to removing the ruby-enum dependency?
2 participants