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

Fixes of keyword renaming #194

Merged
merged 17 commits into from
Jun 26, 2024
Merged

Conversation

zhucaoxiang
Copy link
Contributor

This is an updated version of #160 . Sorry for not looking at this issue for so long. I found that my fork deviates from the official repo and it is difficult to resolve conflicts.

In the past years, my colleagues and I are actually using my fork to avoid issues in renaming keywords. For example, in the following Fortran source (now included in the /examples).

module global

    implicit none
    
    type class2
        integer :: x = 456
    end type class2

    integer :: abc=0
    integer, parameter :: lambda=1
    integer :: with(9)
    
    contains
    
    subroutine is(a)
        implicit none
        integer,intent(in) :: a

        abc = abc + a
        return 
    end subroutine is
end module global

integer function in(a)
    implicit none

    integer, intent(in) :: a

    in = a + 1

    return
end function in

There are module, type, subroutine, and function names conflicting with Python system keywords. The existing public version will report errors (first with the module name). I managed to fix most of them except the derived type, although the derived type name was also working in my old fork.

@jameskermode
Copy link
Owner

Thank you for the contribution. As you will have seen there are failures in the regression tests so this can't yet be merged. Are you working on fixing the failing tests?

@zhucaoxiang
Copy link
Contributor Author

@jameskermode Yes, I noticed the errors and am working on finding solutions.

@smiet
Copy link
Contributor

smiet commented Jun 19, 2024

I see that the test are passing, could this fix be merged soon?

With the numpy 2.0.0 update the diverged branch of @zhucaoxiang is no longer compiling, and maintaining it is suboptimal to directly using yours.

@jameskermode
Copy link
Owner

Yup, I'll take a look. It seems there are now some conflicts, if you are willing to try to resolve them that would speed things up.

@smiet
Copy link
Contributor

smiet commented Jun 19, 2024

I am glad to help!

I cannot see the conflicts myself, I pulled jameskermode:master into my branch of zhucaoxiang:main_off and there were no merge confilcts. Am I missing something?

I don't have write permission to @zhucaoxiang's f90wrap branch. Could you trigger the tests again to see if they still pass?

I will ask @zhucaoxiang for write permission and hopefully lay this issue to rest.

@zhucaoxiang
Copy link
Contributor Author

@smiet I have sent you invitations.

@jameskermode
Copy link
Owner

I'm just going from the GitHub warning above about conflicts. Rebasing on current master should solve it.

@smiet
Copy link
Contributor

smiet commented Jun 19, 2024

There you go, ready to merge I believe!

I see that #220 addresses numpy2.0 support better than what I had done.

@jameskermode
Copy link
Owner

Thank you - I will merge providing the CI tests pass.

@jameskermode
Copy link
Owner

@smiet and/or @zhucaoxiang there are some remaining test failures I am afraid - would you be willing to take a further look?

@smiet
Copy link
Contributor

smiet commented Jun 19, 2024

Could you restart the tests now that #220 has been merged? This should hopefully fix it (on our branch I had the same error before I switched to numpy2.0).

If that doesn't fix it I'll have to get back on it tomorrow.

@jameskermode
Copy link
Owner

You'll first need to update this branch from master to bring in the changes

@jameskermode
Copy link
Owner

Still failing tests I'm afraid

@smiet
Copy link
Contributor

smiet commented Jun 20, 2024

Sorry, Caoxiang ran black over the source and it is really hard to disentangle the logic changes from all the syntax changes.
I found a change I missed and have pushed it now, my apologies. This is on us, and I will keep iterating until all tests pass.

@inoelloc
Copy link
Contributor

diff --git a/f90wrap/f90wrapgen.py b/f90wrap/f90wrapgen.py
index 0c82bfe..a526dfa 100644
--- a/f90wrap/f90wrapgen.py
+++ b/f90wrap/f90wrapgen.py
@@ -497,9 +497,7 @@ end type %(typename)s_rec_ptr_type"""
         self.write_transfer_out_lines(node)
         self.write_finalise_lines(node)
         self.dedent()
-        self.write(
-            "end subroutine %(sub_name)s" % {"sub_name": self.prefix + node.name}
-        )
+        self.write("end subroutine %s" % (sub_name))
         self.write()
         return self.generic_visit(node))

It seems to work with this change

@inoelloc
Copy link
Contributor

A Makefile.meson is missing from the test folder

include ../make.meson.inc

NAME     := keywordr_rename

test: build
	$(PYTHON) tests.py

@zhucaoxiang
Copy link
Contributor Author

Thanks to @inoelloc, all the tests have now been passed. @jameskermode @smiet

@jameskermode
Copy link
Owner

Thanks very much! I have also reviewed the substantive changes and am in principal very happy to accept this contribution. The only thing that gives me a pause for thought is the large number of changes introduced due to reformatting with black which messes up the git history rather and makes it hard to see what the meaningful changes are. Would one of you be willing/able to revert this? If it is not possible I will probably merge, albeit a bit relunctantly.

@zhucaoxiang
Copy link
Contributor Author

zhucaoxiang commented Jun 21, 2024

which messes up the git history rather and makes it hard to see what the meaningful changes are. Would one of you be willing/able to revert this?

I probably did this with automatic formatting. I will check if there is a way to revert it.

@zhucaoxiang
Copy link
Contributor Author

I realized that most of the changes were saved in one commit (ab3798d). There seems no elegant way to revert it unless we go through the two files line by line. I am sure I was using VSCode auto-formating, so there should be no content revised unintentionally. @jameskermode Are you fine with what it is? I am sorry for the inconvenience.

@inoelloc
Copy link
Contributor

@zhucaoxiang Can you confirm that the only changes applied are on name and orig_name variables ?

@smiet
Copy link
Contributor

smiet commented Jun 21, 2024

We apologize for the syntax changes. Unfortunately, isolating them isn't feasible as the initial commit contains substantial amounts of logic changes. @zhucaoxiang and I explored solutions together but found no easy fix. There are more changes relating to single or double _ and __ strings.

We kindly request your acceptance of the PR as-is. And we will ensure that it doesnt' happen again.

@zhucaoxiang
Copy link
Contributor Author

@zhucaoxiang Can you confirm that the only changes applied are on name and orig_name variables ?

That's the main change. But the problem is that I cannot guarantee that's the only change. I modified the files last year.

@inoelloc
Copy link
Contributor

@jameskermode Are you planning to release on PyPI once this PR has been merged ?

@jameskermode
Copy link
Owner

Yes. I will go ahead and merge this then do a PyPI release. Thanks everyone for the work!

@jameskermode jameskermode merged commit bef2881 into jameskermode:master Jun 26, 2024
23 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.

4 participants