Fix bug in Code.get_full_text_info

[sphuber]

Fixes #4079

This method was not tested and so it went unnoticed that it was still
using the _get_folder_pathsubfolder repository method that has been
removed in aiida-core==1.0.0. Direct access to the folder underlying
the node repository is no longer allowed and instead the API should be
used to inspect the available objects, in this case list_objects.

[ramirezfranciscof]

Thanks! I think it all should be good. I only made one specific comment and now two general question:

(1) Why a list of lists? I mean, I understand we don't want to change the interface now, but is there another reason no to have used a dictionary (since this list of lists seems like a "hacked" dict) or even a list of tupples (since this return being info on the node should perhaps not have mutable "info-pieces")?

(2) This was not just the get_full_text_info method that was not being tested, but nothing from the code class correct? Then maybe it could be good to open an issue to discuss what other tests should be making for it?

[sphuber]

(1) Why a list of lists? I mean, I understand we don't want to change the interface now, but is there another reason no to have used a dictionary (since this list of lists seems like a "hacked" dict) or even a list of tupples (since this return being info on the node should perhaps not have mutable "info-pieces")?

This is indeed a remnant of old (sub-ideal) design. In old versions of AiiDA, the ORM classes also included many methods to do a lot of formatting of information such that it is easier, for example, for CLI commands to call them. I think this is not a good design as the formatting of data is not the task of an ORM class but exactly that of a CLI. I have been removing a lot of them, or at the very least, return them in a form where they can still be manipulated somewhat for formatting (i.e. do not return a preformatted string). I totally agree that this current form also makes little sense, so maybe we can deprecate this and remove it for 2.0. I would maybe do this in another PR with all other comparable commands in the ORM interface that should be removed.

(2) This was not just the get_full_text_info method that was not being tested, but nothing from the code class correct? Then maybe it could be good to open an issue to discuss what other tests should be making for it?

Yes, please go ahead

[sphuber]

Ok this is super annoying: the test failed for Py3.5, but I fixed it and pushed the changes. The change shows up here on the PR changes:

filepath = os.path.join(tmpdir.realpath(), filename)  # Python 3.5 does not support Path objects for `join` yet`

But on GHA, the test still fails and it still seems to be running the old code:

==================================== ERRORS ====================================
__________________ ERROR at setup of test_get_full_text_info ___________________

tmpdir = local('/tmp/pytest-of-runner/pytest-0/test_get_full_text_info0')
aiida_localhost = <Computer: localhost-test (localhost-test), pk: 274>

    @pytest.fixture
    def create_codes(tmpdir, aiida_localhost):
        """Create a local and remote `Code` instance."""
        import os
    
        filename = 'add.sh'
>       filepath = os.path.join(tmpdir, filename)

@greschd do you maybe have an idea? Is GHA caching the code? I think I have force pushed very often to fix open PRs and it has never been a problem.

[greschd]

Wow, that's a curious one - I doubt GHA is intentionally caching the code, that would be a curious choice to lay the least.

But looking at the log, it indeed showed

Successfully installed aiida-core
-e git+https://github.com/aiidateam/aiida-core@7dbd51ac88b9a30bf645b224524fd7c411aaaad0#egg=aiida_core

which is (see 7dbd51a) the merge-commit of 37e18bd (your previous commit) into develop.

Maybe there's an internal API somewhere that should tell GHA which commit this PR points to and failed. I've noticed minor glitches on GitHub quite frequently the last few weeks.

How about you make an insignificant edit and force-push again?

[greschd]

Seems like GHA picked the right commit this time around 🎉

[sphuber]

Seems like GHA picked the right commit this time around tada

Except turns out my "fix" was worth exactly nothing 😅

[greschd]

You could also cast to str here, and use the glorious / operator for joining:

filepath = str(dirpath / filename)

[sphuber]

nice! It is clear I have not yet worked with Paths at all. Was going to merge because I am sick of waiting for the builds of this PR, but this is too nice to pass on

[sphuber]

But, then I don't even need the cast right? Or does open also not support Path in 3.5?

[greschd]

You still need the cast, pathlib support across the board only came in 3.6.

[greschd]

I guess we're back to waiting for another build 😅

[codecov]

Codecov Report

Merging #4083 into develop will increase coverage by 0.03%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           develop    #4083      +/-   ##
===========================================
+ Coverage    78.77%   78.80%   +0.03%     
===========================================
  Files          463      463              
  Lines        34414    34414              
===========================================
+ Hits         27106    27116      +10     
+ Misses        7308     7298      -10     

Flag	Coverage Δ
#django	70.71% <80.00%> (+0.03%)	⬆️
#sqlalchemy	71.59% <80.00%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
aiida/orm/nodes/data/code.py	88.94% <80.00%> (+3.69%)	⬆️
aiida/transports/plugins/local.py	80.47% <0.00%> (+0.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b86425f...fa608e9. Read the comment docs.

[sphuber]

I guess that's a no ...