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

Require some utility modules that might come handy in future #449

Closed
bschmalhofer opened this issue Sep 18, 2020 · 7 comments
Closed

Require some utility modules that might come handy in future #449

bschmalhofer opened this issue Sep 18, 2020 · 7 comments
Assignees
Milestone

Comments

@bschmalhofer
Copy link
Contributor

This issue is a hodgepodge of different issues. Some are about adding utilitiy modules, some are about changing the archtecture of OTOBO. Let's collect the list of small utility modules and manage them in a separate module.

Originally posted by @bschmalhofer in #73 (comment)

@bschmalhofer bschmalhofer added this to the OTOBO 10.1 milestone Sep 18, 2020
@bschmalhofer
Copy link
Contributor Author

bschmalhofer commented Sep 18, 2020

Let's decide on a list of utility modules. If accepted these modules should not only be required in bin/otobo.CheckModules.pl but also be listed as recommended modules in the developper manual.

Most of these modules were initially in #73.

  • List::AllUtils, the initial proposal in Issue #73: package names for Net::SMTP::SSL and Convert::BinHex #381 was List::MoreUtils, but List::AllUtils looks like the more convenient solution
  • Const::Fast. readonly variables are a good practice
  • Path::Class, because it provides useful functionality and I like it
  • Syntax::Keyword::Try recommended module for try-catch-blocks
  • String::Dump mostly for development
  • String::Util because it provides many utility function, suggested in Suggestion: Expand list of required and recommended modules #73 wareText::Trim, String::Trim
  • SQL::Interp : looks nifty, might simplify some code
  • File::chmod: for allowing chmod 'u+x' $File

Hint: this list can still be expanded

@svenoe
Copy link
Contributor

svenoe commented Oct 12, 2020

I have no objections against: List::AllUtils, Const::Fast and String::Util.

For the other Modules there are at least some additional questions:

  • Path::Class - I really don't see the use case so much. We are only supporting Linux and hopefully BSD, which don't need any differentiation in their path names. Also opening files is currently done via Kernel/System/Main.pm, which additionally locks the files and so on. I would like to keep that homogeneously, if possible. Open for discussion though.
  • Syntax::Keyword::Try - As a precondition please check, whether we are using some other perlxs modules in an obligatory fashion. Some, for example YAML::XS have explicit fallbacks built into OTOBO. (See for example: Kernel/System/YAML.pm line 141) But if we rely on XS elsewhere anyways, in principle I think it can be added and used. I would not, however, replace any existing eval sections, especially in the core functions of OTOBO, as Kernel::System::Main->Require(), for example. I somewhere read that eval is still twice as fast as Syntax::Keyword::Try, but I don't find the link anymore. It should be only used in the non experimental syntax, also. (We can also have a discussion about the XS stuff in general I guess. Please, Helmut, Bernhard, you have more experience here, share your thoughts.)
  • String::Dump - No objections, but only as a non required module. There are no reasons to use this in productive code, are there?
  • SQL::Interp - Generally it looks nice, yes. I would like to discuss again after seeing the first couple of implementations, though. I'm not sure enough, how much our expectations of its usage align. ;)
  • File::chmod - I'm not sure where you want to use this.

@bschmalhofer
Copy link
Contributor Author

@svenoe , here are my comments regarding the modules in question:

  • Path::Class
    There is some code in OTOBO that deals with manipulation of pathes. A recent example where I used ' Path::Class' is scripts/test/Console/Command/Dev/Tools/Migrate/OTRSToOTOBO.t . But I agree that this needs to be discussed. Personally I'm not a fan of `Kernel::System::Main' .
  • Syntax::Keyword::Try
    DBD::mysqlneeds XS. See https://metacpan.org/source/DVEEDEN/DBD-mysql-4.050/mysql.xs.
  • String::Dump
    Yes, this is only useful for debugging. That module should be categorised as 'dev:encoding' or such
  • SQL::Interp
    I haven't used this module before. I hope it could simplify the current state of how SQL statements are assembled.
  • File::chmod
    I missed that during the implementation of Kernel/System/Console/Command/Dev/Git/InstallHooks.pm. IMHO, chmod 'u+x', $Target is much more readable than
        my $OldMode = (stat($Target))[2];
        my $NewMode = $OldMode | 0100;
        my $RetChmod = chmod $NewMode, $Target;


@svenoe
Copy link
Contributor

svenoe commented Oct 14, 2020

I just had one additional thought, which is - I see it as condition, that at least required packages can be found in at least the Ubuntu package repository. This has been the mainly described way to install OTRS, and I think it needs a really really good reason to give up on even the possibility of installing OTOBO this way. I just checked via https://pkgs.org/ and it is true for all packages but SQL::Interp. It would be a possibility to add those packages to Kernel/cpan-lib, but I guess the general direction is rather to maybe even get rid of that, so for now I'd rather just not use it.

The rest can be used, with those constraints:

  • Syntax::Keyword::Try - please wait until tomorrow, I want a short ok from Stefan.
  • String::Dump - only optional, as discussed.
  • String::Util - please use Text::Trim instead.

@StefanRother-OTOBO
Copy link
Contributor

Hi,

1.) for me is important, that we use the modules first time in 10.1.
2.) Another question is the future plans of Bernhard.
3.) Syntax::Keyword::Try will rather worsen the readability, I'm not sure if this makes sense.

Thanks,

Stefan

bschmalhofer added a commit that referenced this issue Oct 22, 2020
bschmalhofer added a commit that referenced this issue Oct 22, 2020
bschmalhofer added a commit that referenced this issue Oct 22, 2020
bschmalhofer added a commit that referenced this issue Oct 22, 2020
bschmalhofer added a commit that referenced this issue Oct 22, 2020
Add devel:encoding category with String::Dump
bschmalhofer added a commit that referenced this issue Oct 22, 2020
@bschmalhofer
Copy link
Contributor Author

Added the modules as requirements in rel-10_1.

@StefanRother-OTOBO: I skipped Syntax::Keyword::Try. Let's discuss this when exception handling is under discussion.

bschmalhofer added a commit that referenced this issue Jan 9, 2021
This is not a new requirement, as at least LWP::UserAgent depends on it
@bschmalhofer
Copy link
Contributor Author

The POD for Syntax::Keyword::Try had to many times the word experimental. So let's stay conservative and recommend Try::Tiny instead. This has the advantage that it is already required implicitly. In order to document the recommendation I add Try::Tiny as a dependency for 10.0.7 and later. Closing this isssue, as any further requests can be discussed in new issues.

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

No branches or pull requests

4 participants