search

metan-ucw / runltp-ng

Minimalistic LTP testrunner
11 stars 16 forks source link

make: Remove tags target (was: Pass correct --extra to ctags) #35

Closed yangx-jy closed 3 years ago

yangx-jy commented 3 years ago

Signed-off-by: Xiao Yang yangx.jy@fujitsu.com

yangx-jy commented 3 years ago

Hi @metan-ucw @pevik

Is it necessary to add 'make tags' into ci.yml?

pevik commented 3 years ago

Thank you for fixing this. If we want to keep ctags support, it should be also in ci.yml (and you should add Fixes: 766b8db Introduce perltidy and perlcritic")).

But I'd be for removing ctag support. @metan-ucw @cfconrad @wangli5665 @richiejp any opinion?

wangli5665 commented 3 years ago

But I'd be for removing ctag support. @metan-ucw @cfconrad @wangli5665 @richiejp any opinion?

+1, I slightly think that not very necessary to support ctags, because, IMHO it might not part of the project.

wangli5665 commented 3 years ago

Thank you for fixing this. If we want to keep ctags support, it should be also in ci.yml (and you should add Fixes: 766b8db Introduce perltidy and perlcritic")).

BTW, if we decide to support, better to add "/ctags" in the .gitignore file.

yangx-jy commented 3 years ago

+1, I slightly think that not very necessary to support ctags, because, IMHO it might not part of the project.

Yes, ctags is a tool which helps track the definition of function conveniently. I perfer to keep it in Makefile and add it in .gitignore, but don't add it into ci.yml.

pevik commented 3 years ago

+1, I slightly think that not very necessary to support ctags, because, IMHO it might not part of the project.

Yes, ctags is a tool which helps track the definition of function conveniently. I perfer to keep it in Makefile and add it in .gitignore, but don't add it into ci.yml.

Well, if we keep it, I'd also add it to CI. Ideally software should be 100% tested (impossible), thus why refusing an easy test coverage? I meant to remove it, but it does not harm to have it. But keep in mind, that we're planning to move the tool into LTP git soon (probably after fixing https://github.com/metan-ucw/runltp-ng/issues/31 and https://github.com/metan-ucw/runltp-ng/issues/32), and we don't support ctags generation in LTP itself.

yangx-jy commented 3 years ago

Well, if we keep it, I'd also add it to CI. Ideally software should be 100% tested (impossible), thus why refusing an easy test coverage? I meant to remove it, but it does not harm to have it. But keep in mind, that we're planning to move the tool into LTP git soon (probably after fixing #31 and #32), and we don't support ctags generation in LTP itself.

OK, Let's remove it directly.

wangli5665 commented 3 years ago

Reviewed-by: Li Wang <liwang@redhat.com>

yangx-jy commented 3 years ago

LGTM, just please add a note that --extras option is was wrong, but instead of fixing it to --extra we decided to remove the target.

Done.

metan-ucw commented 3 years ago

Looks good, acked.