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

Create Future deprecation warning for ODM2 modules #145

Closed
wants to merge 3 commits into from

Conversation

lsetiawan
Copy link
Member

@lsetiawan lsetiawan commented Feb 5, 2018

NOT READY TO BE MERGED

Overview

This is a test of the deprecation for ODM2 module folder.

@emiliom
Copy link
Member

emiliom commented Feb 5, 2018

@lsetiawan Thanks for taking these steps. I won't be able to review this until late today or tomorrow (it's 12 files changed, and it's more complicated than usual!).

In the meantime, in the warning text, the word "deprecated" is misspelled everywhere as "depricated". You can go ahead and fix that.

@emiliom
Copy link
Member

emiliom commented Feb 13, 2018

@lsetiawan I have a question for you, that may be a recommendation. I can see that you edited in place 5 modules in the ODM2 directory we're deprecating, and copied/created versions of those modules (plus services/__init__.py) in the new tree hierarchy that starts at the base path level. But as far as I can tell the relocated files (or copies) were created as "new" files, meaning -- and this is the question -- that they won't retain their git commit history; all the history will remain with the files in the old, deprecated ODM2 directory, which will eventually be removed.

Is that interpretation correct? If so, can you re-do this PR to implement exactly the same changes you've submitted, but done in such a way that you maintain the full git history in the new files? ie, we'd still be able to do a git diff to see what's changed relative to any commit of our choice.

Notes to self (my progress status; @lsetiawan, ignore this):

  • Of the 12 modules (files) in this PR, 6 are simple "moves" from former ODM2 directory/hierarchy to the base package level; 5 of those are in the services directory, and the other one is models.py
  • ODMconnection.py was already at the base package level, so it did not need to be moved. Only one simple change was made, to an import statement, and it looks Ok
  • That means that I only have 5 modules left to review -- the ones left in the interim ODM2 folder

@lsetiawan
Copy link
Member Author

Is that interpretation correct? If so, can you re-do this PR to implement exactly the same changes you've submitted, but done in such a way that you maintain the full git history in the new files? ie, we'd still be able to do a git diff to see what's changed relative to any commit of our choice.

Your interpretation is correct... I'm not sure how to do that in git.

@emiliom
Copy link
Member

emiliom commented Feb 13, 2018

Is that interpretation correct? If so, can you re-do this PR to implement exactly the same changes you've submitted, but done in such a way that you maintain the full git history in the new files? ie, we'd still be able to do a git diff to see what's changed relative to any commit of our choice.

Your interpretation is correct... I'm not sure how to do that in git.

Ok. Tell me if I'm wrong, but this sequence (order) is what I have in mind:

  1. use git mv to move all the files you're moving from ODM2 to the base path
  2. "manually" copy these files back into the ODM2 directory (it looks like git has no git cp command?)
  3. now apply your edits to each set of modules, just as you did before. But now the git history will stay with the modules that will remain after the ODM2 is removed, not with the modules that will disappear

@lsetiawan
Copy link
Member Author

Okay. I will try that on Friday. Thanks.

@lsetiawan
Copy link
Member Author

Please test this branch anyhow to make sure that the new and old functions actually work if you have some time. Thanks.

@emiliom
Copy link
Member

emiliom commented Feb 13, 2018

Okay. I will try that on Friday. Thanks.

So, does my suggestion make sense to you?? You're the git expert here, not me!

Please test this branch anyhow to make sure that the new and old functions actually work if you have some time. Thanks.

I'll try, but no promises ...

@lsetiawan
Copy link
Member Author

lsetiawan commented Feb 13, 2018

So, does my suggestion make sense to you?? You're the git expert here, not me!

Yepp. It makes sense, just never done it.

@emiliom
Copy link
Member

emiliom commented Feb 16, 2018

@lsetiawan are you planning to implement today the PR changes I recommended?

Just trying to see what to expect today. I have a meeting at 2pm at Suzzallo.

@lsetiawan
Copy link
Member Author

@emiliom I am about to do a PR, but it seems like, that didn't workout... The change history is still exactly the same.. hmmm.

@lsetiawan lsetiawan mentioned this pull request Feb 16, 2018
@emiliom
Copy link
Member

emiliom commented Feb 16, 2018

Hmm

@lsetiawan
Copy link
Member Author

Closing this PR, replaced by #147

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