Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11735 )

Change subject: [build] Centralize project version to version.txt
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/11735/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11735/1//COMMIT_MSG@15
PS1, Line 15: to include updating the example project poms.
> Why bother? The examples will still work even if they're on a slightly olde
I think it is best if people just starting with Kudu use the latest versions 
given how fast Kudu is developing and improving. We could run this script a bit 
differently to include the previous release version I suppose so we never have 
an unreleased or snapshot version listed.


http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py
File python/setup.py:

http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@46
PS1, Line 46:         
r"^(?P<version>(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+))(?:-(?P<label>.+))?",
> Please provide some examples of what this matches against in a comment. Als
I name the subgroups because it's sort of self documenting the regex. That way, 
it's very easy to see the version structure I am matching without examples.

I added documentation with examples, but it feels a bit redundant.


http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@48
PS1, Line 48:         re.M,
> re.MULTILINE is way more understandable. But why do we need it? version.txt
Done


http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@52
PS1, Line 52:     raise RuntimeError("Unable to parse version string.")
> Can you include the contents of version_file in this case?
Done


http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@54
PS1, Line 54: def write_version_py(filename=os.path.join(setup_dir, 
'kudu/version.py')):
> What is version.py used for? It seems like it dumps a __version__ variable
Honestly I don't know. This is a python convention that appears common but I 
didn't dig into understanding.


http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@189
PS1, Line 189:     version=parse_version_and_label()[0],
> So it's not possible to get here without first calling write_version_py() o
Done



--
To view, visit http://gerrit.cloudera.org:8080/11735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654be8657ae3dc2f193056484b5d0743aa1ce95
Gerrit-Change-Number: 11735
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Attila Bukor <[email protected]>
Gerrit-Reviewer: Grant Henke <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Comment-Date: Fri, 19 Oct 2018 17:21:18 +0000
Gerrit-HasComments: Yes

Reply via email to