Skip to content
Snippets Groups Projects

SW-824: Resolve SW-824

Merged SW-824: Resolve SW-824
SW-824 into LOFAR-Release-4_0
1 unresolved thread
Merged Jorrit Schaap requested to merge SW-824 into LOFAR-Release-4_0
1 unresolved thread

Closes SW-824

Merge request reports

Approval is optional

Merged by Jorrit SchaapJorrit Schaap 5 years ago (Oct 10, 2019 12:07pm UTC)

Merge details

  • Changes merged into LOFAR-Release-4_0 with 8e4a7559.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
LCS/PyCommon/cep4_utils.py 100644 → 100755
99 99 dockerized_cmd += cmd
100 100 return dockerized_cmd
101 101
102 def get_cep4_available_cpu_nodes():
102 def get_cep4_available_cpu_nodes(include_allocated_nodes=False):
  • Auke Klazema
    Auke Klazema @klazema ·
    Contributor

    Here you introduce a so-called Flag Argument. This allows this function to show two different behaviors. This would be a violation of the Single Responsibility Principle (SRP). From a readability point this is less of a problem when you always use this function with a named argument. It is then clear that you either include or not include the allocated nodes. But this requires discipline that is not checked by tools. So it will be forgotten and that would make the a little bit less readable.

    If you have two functions that clearly state what it does, and with a refactoring extract the common code out to a function that then accepts the specific part as argument (list of node states) you have the responsibilities nicely separated.

  • Jorrit Schaap changed this line in version 2 of the diff

    changed this line in version 2 of the diff

  • Jorrit Schaap
    Jorrit Schaap @schaap ·
    Author Maintainer

    agreed. See changes in last commit.

  • Please register or sign in to reply
  • Auke Klazema
    Auke Klazema @klazema ·
    Contributor

    Code changes otherwise look good. I do miss a test for the additional behavior when including allocated nodes.

  • Auke Klazema added process_review label

    added process_review label

  • Jorrit Schaap added 2 commits

    added 2 commits

    Compare with previous version

  • Auke Klazema
    Auke Klazema @klazema ·
    Contributor

    Not really liking the filtering of the input to make it work. I would rather raise an exception on bad input. But that being said, go for the merge.

  • Jorrit Schaap added 1 commit

    added 1 commit

    • d6efc8f4 - SW-824: processed review comments, better way of input checking.

    Compare with previous version

  • Jorrit Schaap merged

    merged

  • Jorrit Schaap mentioned in commit 8e4a7559

    mentioned in commit 8e4a7559

    • Please register or sign in to reply