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 singular in full-service path #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

palday
Copy link
Member

@palday palday commented Jun 7, 2022

Legolas schema use singular by convention and Onda follows this convention.

I'm unsure whether this should be considered a breaking change and hence what the necessary version bump is. In some sense it's a bug fix, but it definitely changes behavior in a non compatible way.

Legolas schema use singular by convention and Onda follows this convention.
@palday palday requested a review from kleinschmidt June 7, 2022 13:41
@jrevels
Copy link
Member

jrevels commented Jun 7, 2022

I'm unsure whether this should be considered a breaking change and hence what the necessary version bump is.

i'd consider it breaking (and thus entail a major bump) even though it's a bugfix since the likelihood of downstream users/code hardcoding these extensions is pretty high

given that it's a breaking change anyway I think I'd be a fan of just removing the magical extension handling bits altogether and changing the interface from e.g. signal_prefix to signal_file_path

better to just let the caller fully choose their own file paths/names IMO

Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

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

I do think that for the "full service" option it's good to have defaults, but yeah let's make them file names instead of prefixes (if we're making a breaking change anyway)

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