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

Use Pronto YML For Config #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Phillita
Copy link

Motivation

To reduce the number of config files needed for pronto integrations. Pronto has a config that can handle plugin config.

Changes

  • Added a new config class to handle all config for the gem
  • Deprecated use of the old config file
    • It still works but a warning will be issued
  • Handled a bunch of Rubocop warnings

Added a new config class. Handle new config from the pronto.yml. Added warning when using the old config. More Rubocop warnings fixed. Added some quality of life improvements to the specs.
Remove constants that only the config needs. Move config to attr_reader
Copy link
Owner

@kevinjalbert kevinjalbert left a comment

Choose a reason for hiding this comment

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

👋🏻 It has been a while since I've done anything with this repo, or even using pronto 😅.

What you are proposing here makes sense though, remove the bespoke config as Pronto has better support for configs of plugins.


I'm thinking it would better to just make a breaking change and adopt the pronto config approach.

Request changes:

  • I noticed that tests are emitting the deprecation warning you put in. We would simplify config logic (to the new approach) and just remove the old way.
  • Update the README to reflect the new config approach (link to official pronto docs for an example)

🙇🏻 Much appreciated on your contributions, looking forward to the changes.


Also, just notes to me.

It took me a bit to 'run the tests' of this PR, was hitting some node issue with stylelint (and the meow package), had to go to the latest node version. Could have a better 'how to test/contribute' in the README.

The CI for this repo doesn't work anymore and probably should just be a GitHub Action now.

@Phillita
Copy link
Author

Phillita commented Jul 4, 2024

👋🏻 It has been a while since I've done anything with this repo, or even using pronto 😅.

What you are proposing here makes sense though, remove the bespoke config as Pronto has better support for configs of plugins.

I'm thinking it would better to just make a breaking change and adopt the pronto config approach.

Request changes:

  • I noticed that tests are emitting the deprecation warning you put in. We would simplify config logic (to the new approach) and just remove the old way.
  • Update the README to reflect the new config approach (link to official pronto docs for an example)

🙇🏻 Much appreciated on your contributions, looking forward to the changes.

Also, just notes to me.

It took me a bit to 'run the tests' of this PR, was hitting some node issue with stylelint (and the meow package), had to go to the latest node version. Could have a better 'how to test/contribute' in the README.

The CI for this repo doesn't work anymore and probably should just be a GitHub Action now.

Thank you for replying! This is one of many pull requests I've recently submitted to upgrade Pronto plugins. I just wanted to let you know you're the first to reply.
I'll get your changes introduced as soon as I can. Also, thank you for the original gem code!

Remove deprecated code in favour of the new config. Updated the readme. Fixed specs.
@Phillita
Copy link
Author

Phillita commented Aug 2, 2024

@kevinjalbert Sorry about the delay. I was away on vacation and had been unplugged. I've made the changes as you requested!

@Phillita Phillita changed the title Deprecate config use pronto yml Use Pronto YML For Config Sep 11, 2024
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.

2 participants