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

Fix Commit history of API changes #674

Closed
wants to merge 3 commits into from

Conversation

Smit-create
Copy link
Member

@Smit-create Smit-create commented Dec 20, 2022

Fixes #671

@coveralls
Copy link

coveralls commented Dec 20, 2022

Coverage Status

Coverage: 94.038% (-1.3%) from 95.347% when pulling d1e5919 on Smit-create:i-671 into 9ffb7d2 on QuantEcon:main.

@Smit-create
Copy link
Member Author

The commit history issues seem to be resolved and it can be verified on my fork's branch: https://github.com/Smit-create/QuantEcon.py/tree/i-671/quantecon

@@ -6,7 +6,7 @@
"""
import numbers
import numpy as np
from ..compute_fp import _compute_fixed_point_ig
from quantecon._compute_fp import _compute_fixed_point_ig
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be from .._compute_fp?

Suggested change
from quantecon._compute_fp import _compute_fixed_point_ig
from .._compute_fp import _compute_fixed_point_ig

(Several other places.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Relative imports are often confusing so IMHO it's better to use absolute imports.

Copy link
Member

Choose a reason for hiding this comment

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

Is it just a matter of exposition? I thought it made some real difference in some occasions (but I don't know what it would be).

But one general idea is that one should avoid hard coding: If we wanted to change the library name (although it is very unlikely to happen), the part would have to be modified.

Copy link
Member

Choose a reason for hiding this comment

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

@mmcky You made some argument about this in #137 (comment) (long time ago..). Would that apply here?

@Smit-create Smit-create marked this pull request as draft December 31, 2022 18:02
@oyamad
Copy link
Member

oyamad commented Jan 2, 2023

What is the status of this PR? We can discuss the "from quantecon.XXX versus from ..XXX" issue later.

@Smit-create
Copy link
Member Author

I have rebased this on the top of the latest main branch.
Steps to verify this PR:

  1. Checkout a new branch from main say qe_new_main_check
  2. Merge this branch on my fork to this new branch by: git merge Smit-create/i-671.
  3. Please try to check the commit history of individual files mainly those of _name.py by git log _name.py.

Please try these steps to verify and when both @oyamad and @mmcky agree we can look forward to merging this.

Somehow there is some overriding to commit history because of merge conflicts and so the commit history of these files is getting lost.

@oyamad
Copy link
Member

oyamad commented Jan 2, 2023

As you force-pushed it seems that the commit histories have been lost again, e.g., History, Blame. I had seen them certainly there before you did force push (resolve the conflicts?).

@oyamad
Copy link
Member

oyamad commented Jan 2, 2023

@Smit-create Can you share your script to change the file names? (I guess you didn't type commands manually one by one?)

@Smit-create
Copy link
Member Author

Renaming files using this:

Python script

import subprocess

# add full path
ROOT_DIR_PATH = '/Users/thebigbool/repos/Quantecon.py/quantecon'

files = ['lae.py', 'gridtools.py', 'estspec.py', 'ivp.py',
'arma.py', 'lqnash.py',
'kalman.py', 'lqcontrol.py', 'inequality.py',
'lss.py', 'dle.py', 'matrix_eqn.py', 'discrete_rv.py',
'rank_nullspace.py', 'graph_tools.py', 'robustlq.py', 'ce_util.py',
'ecdf.py', 'filter.py', 'compute_fp.py', 'quadsums.py']


for file in files:
    _file = "_" + file
    cmd = f"git mv {file} {_file}"
    process = subprocess.run(cmd, shell=True, cwd=ROOT_DIR_PATH)
    if process.returncode != 0:
            print(f"Command failed: {cmd}")
            exit(1)

@Smit-create
Copy link
Member Author

Closing in favor of #682

@Smit-create Smit-create closed this Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Commit History?
3 participants