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 NamingStyle adapter to enforce format for added features. #880

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bkeepers
Copy link
Collaborator

As suggested by @LandonSchropp in #879, this adds the ability to constrain feature names to a naming style.

require 'flipper/adapters/naming_style'

Flipper.configure do |config|
  config.use Flipper::Adapters::NamingStyle, :snake # or :camel, :kebab, :screaming_snake, or a Regexp
end

# This will work because the feature key is in snake_case.
Flipper.enable(:snake_case)

begin
  # This will raise an error because the feature key is in CamelCase.
  Flipper.enable(:CamelCase)
rescue Flipper::Adapters::NamingStyle::InvalidFormat => e
  puts "#{e.class}: #{e.message}"
end

Output:

Flipper::Adapters::NamingStyle::InvalidFormat: Feature key "CamelCase" does not match format /^[a-z0-9]+(_[a-z0-9]+)*$/

@bkeepers bkeepers requested a review from jnunemaker August 22, 2024 13:35
invalid: %w[CamelCase cobraCase double__underscore],
},
kebab: {
valid: %w[kebab kebab-case kebab-case-1 htt-party],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I approve of these kebabs. 🎉

Copy link
Collaborator

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

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

This is sick. So clean. Thanks for whipping this together. I'm good as is. Random questions below:

  1. Any thoughts on if/how we should communicate the error raised from the flipper-ui? Should we rescue this error and add validation errors to the form or something? I could see someone being confused about an exception/error page with no details of why it is popping up. If we somehow rescue this error and explain maybe that would be enough?
  2. Any thoughts on what should happen in cloud sync when this error is hit? I think most of the cloud syncing avoids raising anything so I'm wondering if they'd not get feedback.

I'm good with leaving as is for now but part of me wants to add this by default with snake case in the future.

@LandonSchropp
Copy link

Awesome!

Any thoughts on if/how we should communicate the error raised from the flipper-ui? Should we rescue this error and add validation errors to the form or something?

IMO, the best UX would be an inline error message that displays once the user has blurred the input. The second-best would be a banner/inline error if the user tries to submit the form. 😄

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.

3 participants