Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative:
There may even be some fancy logic in Make itself.
@evgeni thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this breaks on
foreman-tasks
where the domain isforeman_tasks
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are smarter ways to do
basename
in Make I think, but we really should not break tasks (which has that weird naming, but here we are).The Ruby line seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is that the filename also matches the gem name so it shouldn't be a difference. In foreman-tasks it's also
foreman-tasks.gemspec
so there it wouldn't be a difference: both would be wrong. It'd be so much better if we renamed the gem toforeman_tasks
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I don't have good experience with renaming gems so I'd like to avoid it if possible. Could we rename the domain instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you can't do that since
-
is not a valid character. It will fail here. I also don't know if the domain is used somewhere else.Note that renaming the gem would also make it follow our other conventions and we don't need to special case it everywhere. Things like https://github.com/theforeman/puppet-foreman/blob/56404b20402960ff32ec983ce06c7c67517e2ce1/manifests/plugin/tasks.pp#L18 could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare plugin_apipie to gettext.
I think the question is whether we want to support a dash at all. I think
foreman-tasks
is the only exception so I'm leaning to fixing the exception rather than allowing everything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened https://projects.theforeman.org/issues/35813 to track the foreman-tasks rename.