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

Remove reference to commit IDs #622

Merged
merged 7 commits into from
Sep 18, 2017
Merged

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Aug 15, 2017

This PR removes the reference to the commit ID for taxcalc, ogusa and btax as requested in #612. Since all of these packages are versioned, there is no need to pin them to the version and specific commit.

@hdoupe hdoupe changed the title Remove Reference to Commit codes Remove Reference to Commit IDs Aug 15, 2017
@hdoupe hdoupe changed the title Remove Reference to Commit IDs Remove reference to commit IDs Aug 15, 2017
@hdoupe
Copy link
Collaborator Author

hdoupe commented Aug 15, 2017

Then link still points towards https://github.com/open-source-economics/Tax-Calculator.

How do I edit the html code so that it points towards:
https://github.com/open-source-economics/Tax-Calculator/tree/0.9.2

In Python, I would do something like this:

<p><a href="https://github.com/OpenSourcePolicyCenter/Tax-Calculator/tree/{}".format(taxcalc_version)>Version {{ taxcalc_version }} - GitHub</a></p>

@GoFroggyRun @brittainhard

@martinholmer
Copy link
Contributor

@hdoupe asked:

How do I edit the html code so that it points towards:
https://github.com/open-source-economics/Tax-Calculator/tree/0.9.2

I don't think this needs to be an HTML link does it?
We just wan the correct release number (for example, 0.9.2).

@hdoupe
Copy link
Collaborator Author

hdoupe commented Aug 15, 2017

@martinholmer I think that the link should point towards the version that the webapp is using. This PR shows the version of the Tax-Calculator that TaxBrain is using but when you click the link it takes you to
https://github.com/open-source-economics/Tax-Calculator

instead of

https://github.com/open-source-economics/Tax-Calculator/tree/0.9.0

@martinholmer
Copy link
Contributor

@hdoupe said:

I think that the link should point towards the version that the webapp is using. This PR shows the version of the Tax-Calculator that TaxBrain is using but when you click the link it takes you to

https://github.com/open-source-economics/Tax-Calculator

instead of

https://github.com/open-source-economics/Tax-Calculator/tree/0.9.0

OK. If you have the version/release number (for example, 0.9.0) in a variable, then can't you construct the second URL above in a variable called (say) unique_url.taxcalc_release using Python string operations? And then you can refer to that as the hyperlink in the HTML code.

Do you know where in the code the variable unique_url.taxcalc_vers is set?

@martinholmer
Copy link
Contributor

@hdoupe, #622 is good progress, but can we add just two more things?

First, everywhere we have the phase "created_on|date", can we add the time zone, UTC.
That UTC string would be part of the HTML code, right?
This phrase occurs three time in the source code, as best as I can tell.

And second, can the sentence containing the "created_on|date" phrase, read like this?

These results were generated by TaxBrain version {{ ... }} on ...date... using Tax-Calculator version {{ ... }}

I think you can get the TaxBrain version by calling _version.get_versions()['version'].
Is that correct?

@brittainhard

@hdoupe
Copy link
Collaborator Author

hdoupe commented Aug 17, 2017

@martinholmer Sounds good to me. I'll add these suggestions.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Aug 22, 2017

@martinholmer said

And second, can the sentence containing the "created_on|date" phrase, read like this?

These results were generated by TaxBrain version {{ ... }} on ...date... using Tax-Calculator version {{ ... }}

This seems sensible to me but I got the following result:
screen shot 2017-08-22 at 2 37 06 pm

I think there may be an issue with how the version is set. Lines 21-24 of taxbrain_server/_version.py are

# these strings are filled in when 'setup.py versioneer' creates _version.py
tag_prefix = ""
parentdir_prefix = "taxcalc-"
versionfile_source = "taxcalc/_version.py"

and taxbrain_server/__init__.py is blank even after running the installation process.

But in taxcalc the corresponding lines are

# these strings are filled in when 'setup.py versioneer' creates _version.py
tag_prefix = ""
parentdir_prefix = "taxcalc-"
versionfile_source = "taxcalc/_version.py"

and __init__.py contains

from taxcalc._version import get_versions
__version__ = get_versions()['version']
del get_versions

I thought that maybe you had to run python setup.py versioneer to set up the version reporting function in taxbrain_server/__init__.py. It did do this, but it doesn't refer to the correct version. Lines 21-24 of taxbrain_server/_version.py became

tag_prefix = ""
parentdir_prefix = "taxbrain_server-"
versionfile_source = "taxbrain_server/_version.py"

and __init__.py now contains

from ._version import get_versions
__version__ = get_versions()['version']
del get_versions

However, when I tried to retrieve the version:

In [1]: from _version import get_versions

In [2]: get_versions()
Out[2]: {'full': '', 'version': 'unknown'}

But when I do the same thing for Tax-Calculator, I get:

In [1]: from _version import get_versions

In [2]: get_versions()
Out[2]: 
{'full': '320d7b107c5a0121e5add720ca8d6cbd0e99394c',
 'version': '0.9.2-85-g320d7b10'}

It looks like taxbrain_server/_version.py is not finding the correct version of the webapp. I'm not very familiar with versioneer but I can try to figure this out. Does anyone have any idea what's going on here or where I should start looking?

@brittainhard

@martinholmer
Copy link
Contributor

@brittainhard, How does one get the version of TaxBrain (that is, the webapp-public release number, 1.y.z)?
It has been almost three weeks since @hdoupe asked how to do this. See a description of his problem here in 622. Can you give him some advice?

@brittainhard
Copy link
Contributor

@martinholmer i'll take a look.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 8, 2017

Thanks @brittainhard

@brittainhard
Copy link
Contributor

@hdoupe @martinholmer there are a couple of solutions to this.

Since TaxBrain isn't a python package, the only way to know its version is by getting the latest git tag. In the app, you would have to make a subprocess call like this:

import subprocess
import shlex
call = subprocess.Popen(
    shlex.split("git describe --abbrev=0 --tags"),
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE
)
result = call.communicate()
print(result[0]) # The tag

However, I think it is a better idea to set a string constant variable that provides the version. In the settings file you could add TAXBRAIN_VERSION="v1.0.1". Then, in the views file you can add from django.conf import settings and then to get the version taxbrain_version = settings.TAXBRAIN_VERSON.

Since the webapp version isn't something that changes often, it's not worth it to make a programmatic call to get the current version every time a page is loaded. I suggest going with the second option.

Let me know if you have any questions.

@martinholmer I appreciate the ping on this one. There's lots of stuff to keep track of, and some things just slip through the cracks.

@martinholmer
Copy link
Contributor

@brittainhard said:

Since TaxBrain isn't a python package, the only way to know its version is by getting the latest git tag. In the app, you would have to make a subprocess call like this:

CODE FRAGMENT

However, I think it is a better idea to set a string constant variable that provides the version. In the settings file you could add TAXBRAIN_VERSION="v1.0.1". Then, in the views file you can add from django.conf import settings and then to get the version taxbrain_version = settings.TAXBRAIN_VERSON.

Thanks for the helpful information.

But I have one question. If versioneer doesn't provide the TaxBrain version, what does it do in webapp-public? Put another way: Why don't we eliminate versioneer from the webapp-public repo?

@hdoupe

@brittainhard
Copy link
Contributor

brittainhard commented Sep 13, 2017

@martinholmer I know there is a versioneer script for the deploy folder (since it used to be a separate repository) but I don't think there is one for webapp. In that case its probably wise to remove the deploy versioneer script.

I think that since there is no standard release process for webapp (just version tagging with a string) there wouldn't be much use for versioneer.

EDIT: By "standard release process" I mean a release process that uses conda or pypi and a setup.py script.

@martinholmer
Copy link
Contributor

@brittainhard said:

I know there is a versioneer script for the deploy folder (since it used to be a separate repository) but I don't think there is one for webapp. In that case its probably wise to remove the deploy versioneer script.

I think that since there is no standard release process for webapp (just version tagging with a string) there wouldn't be much use for versioneer.

By "standard release process" I mean a release process that uses conda or pypi and a setup.py script.

@brittainhard, Thanks for the explanation.
@hdoupe, Seems like you should add this to your remove-unneed-code list.

@MattHJensen

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 13, 2017

Thanks @martinholmer and @brittainhard

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 14, 2017

@martinholmer @MattHJensen Do you think that we should use "Webapp-Public version 1.0.1" or
"TaxBrain version 1.0.1" when the version is displayed? The version applies to the entire website right now including the CCC. So, it would make more sense to me to reference "Webapp-Public" instead of "TaxBrain."

@martinholmer
Copy link
Contributor

@hdoupe asked:

Do you think that we should use "Webapp-Public version 1.0.1" or
"TaxBrain version 1.0.1" when the version is displayed? The version applies to the entire website right now including the CCC. So, it would make more sense to me to reference "Webapp-Public" instead of "TaxBrain."

Good points. However, I don't think anybody but the developers will understand "Webapp-public".
Why not just "Webapp" or "webapp" depending on whether you're going to use "Tax-Calculator" or "taxcalc"?

I assume that in your question, you meant to refer to 1.0.2, right? Isn't that the next webapp version that will be using taxcalc 0.10.2 (or 0.10.x if you find more taxcalc bugs)?

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 14, 2017

However, I don't think anybody but the developers will understand "Webapp-public".
Why not just "Webapp" or "webapp" depending on whether you're going to use "Tax-Calculator" or "taxcalc"?

That makes sense. I'm going with "Tax-Calculator." I'll use "Webapp" unless @MattHJensen objects. We could change it in the future, too.

I assume that in your question, you meant to refer to 1.0.2, right? Isn't that the next webapp version that will be using taxcalc 0.10.2 (or 0.10.x if you find more taxcalc bugs)?

Yep, 1.0.2 is the right version. Nice catch.

@MattHJensen
Copy link
Contributor

@hdoupe, my preference is "PolicyBrain." I also plan to rename the repo, and eventually we ought to separate ospc.org content to a separate repo.

@martinholmer @GoFroggyRun

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 14, 2017

@MattHJensen Ok sounds good to me.

@MattHJensen
Copy link
Contributor

I just changed the repo name 💥

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 15, 2017

@MattHJensen Awesome, I'll make the path changes in this PR, too.

@MattHJensen
Copy link
Contributor

MattHJensen commented Sep 15, 2017

Thanks. All of the old paths will remain active, so I do not believe that the update is urgent. But it is certainly good to do.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 15, 2017

@MattHJensen Ah, right. Never mind.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 15, 2017

@brittainhard @GoFroggyRun This PR is ready for review.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 18, 2017

@brittainhard @GoFroggyRun This PR is ready for review.

Nevermind, I'll take a look at this.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 18, 2017

Ok, now this is ready for review.

@GoFroggyRun
Copy link
Contributor

@hdoupe This PR looks good to me. But you might want to take a look at those merging conflicts resulting from recent merge of #641 .

@hdoupe
Copy link
Collaborator Author

hdoupe commented Sep 18, 2017

@GoFroggyRun Thanks for the review. I'll take a look at this.

@brittainhard
Copy link
Contributor

LGTM, merging.

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.

5 participants