Skip to content

fix: Linting fixes, and code fixes #246

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

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

Robin-Van-de-Merghel
Copy link
Contributor

@Robin-Van-de-Merghel Robin-Van-de-Merghel commented Mar 16, 2025

I went in a journey to understand the code. At the same time I tried to fix some linting and documentation problems.

Most changes are made by linters.

  • Improved .pylintrc to maybe later have a stricter linting
  • Few self.log.<logtype> were incorrect (wrong number of parameters), and didn't raise any error in python but did not print out as we desired
  • Adding documentation in some functions (need a review in case I did not understand well):
    • pilotTools.py: pythonPathCheck, safe_listdir, ObjectLoader, getCommand, __checkSecurityDir
    • proxyTools.py: getVO (the others functions, I did not understand them well)
  • Remove unused parameters:
    • pilotTools.py: CommandBase,
  • Still some functions that have unused parameters, and I did not know if we needed them:
    • pilotCommands.py: _setNagiosOptions
    • pilotTools.py: debug, error, warn, info
  • Changed __checkSecurityDir because I think it is not right to test two (or more) arbitraries folders coming from a configuration, before testing if the environment (for whaterver reason) has already a environment variable associated to it (also added at the same time __setSecurityDir)
  • Remove empty test TestSimplePilotLogger (I did not see any use)

Previous pylint score: 5.42/10

New pylint score: 9.33/10

There are still errors like empty documentation. Others seems to come from python2/3 compatibility.

Comment on lines 118 to 120
Raises:
x: In case there is an error while removing duplicates in the PYTHONPATH
x: In any other issue
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The x name comes from the code itself: except Exception as x: (very general).

We can modify it so that exceptions are more precise.

Or modify it to:

    """Checks where python is located
    Raises:
        Error: In case there is an error while removing duplicates in the PYTHONPATH or for any other issue
    """

Or remove this part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am for more precision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i'm working on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is better

Idk if we have to detail each error that can appear (OS, env, ...) with multiple catches

@Robin-Van-de-Merghel Robin-Van-de-Merghel changed the title Linting fixes, and code fixes fix: Linting fixes, and code fixes Mar 17, 2025
@@ -703,7 +725,7 @@ def sendMessage(url, pilotUUID, wnVO, method, rawMessage):
class CommandBase(object):
"""CommandBase is the base class for every command in the pilot commands toolbox"""

def __init__(self, pilotParams, dummy=""):
def __init__(self, pilotParams):
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this. Do the integration tests work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First lines of pilotCommands.py, we have:

The constructor must call the superclass constructor with the PilotParams object and the command name as arguments, e.g.::

    class InstallDIRAC(CommandBase):

      def __init__(self, pilotParams):
        CommandBase.__init__(self, pilotParams, 'Install')
        ...

I guess this comment is wrong, because the name used is <commandClass>.__class__.__name__. And this is the only reference of using that second parameter dummy. All super(<CommandClass>, self).__init()__ use only one parameter.

I am trying to compare what I see (no call of this parameter) with what it does in a real integration test, but I need to configure it.

Note if we remove it, do the same in line 735.

@Robin-Van-de-Merghel
Copy link
Contributor Author

I still need to fix bugs. I'm on it today.

@Robin-Van-de-Merghel
Copy link
Contributor Author

Robin-Van-de-Merghel commented Mar 18, 2025

One of my errors in the pytests is this one.

Recently they changed how they wanted to work with X509 for SSL requests: giving context instead of X509.

I doubt that it is retro-compatible, but I can't verify it right now on my setup (I have python2, but not cmvfs). If it is not it is an issue, and we have to fix the version of this module.

(The issue occurs here)

@fstagni
Copy link
Contributor

fstagni commented Mar 18, 2025

Which of your changes in this PR triggers that pytest errors? As pytest is failing for all python versions here

@Robin-Van-de-Merghel Robin-Van-de-Merghel force-pushed the master branch 3 times, most recently from 94a25a9 to 8fe7080 Compare March 18, 2025 12:40
@fstagni fstagni merged commit 746d6b4 into DIRACGrid:devel Mar 18, 2025
11 checks passed
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.

2 participants