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

Reformat and clean up Name definitions #909

Merged
merged 13 commits into from Mar 19, 2018

Conversation

@jougs
Copy link
Contributor

@jougs jougs commented Mar 12, 2018

This PR removes the (mostly wrong and useless) comments from the headers that define Name objects, cleans up inconsistent definitions/declarations and removes unused Names.

It also adds a regression test to check consistent definition/declaration and also that all Names defined are actually used.

jougs added 3 commits Mar 12, 2018
This tests if the definitions and declaration of const Names are
consistent and that all names defined are actually used.
@jougs jougs mentioned this pull request Mar 12, 2018
@jougs jougs requested a review from heplesser Mar 12, 2018
@jougs jougs added this to the NEST 2.16 milestone Mar 12, 2018
@jougs jougs changed the title reformat names header Reformat and clean up Name definitions Mar 12, 2018
@jougs jougs force-pushed the jougs:feature/reformat-names-header branch from f2d46ca to 0443bc3 Mar 13, 2018

from subprocess import check_output

names_used_raw = check_output(["grep", "-ro", "names::[A-Za-Z0-9_]*", source_dir])

This comment has been minimized.

@jougs

jougs Mar 13, 2018
Author Contributor

This could be refactored using os.walk, iterating files and performing regex matching directly in Python. I settled on the subprocess/grep solution, as it requires less code. However, I'm open to discuss this decision and implement the pure Python solution if the reviewer thinks that that a self-contained implementation is better.

Copy link
Contributor

@heplesser heplesser left a comment

@jougs Thanks for working on this! I added some suggestions concerning details of the python code for the test.

I am uncertain about dropping all the comments. Maybe imprecise and outdated comments (and quite a number are) are worse than no comments; and with a decent IDE you can quickly check where a name is used. Should be discuss this for a final decision at the next NEST ODVC?

@@ -0,0 +1,101 @@
# -*- coding: utf-8 -*-
#
# unused_names.py

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

I would suggest test_unusued_names.py as name and placing this in unittests, as the test is not directly motivated by a reported issue.

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

Sounds reasonable. I've renamed/moved the file in 69456a5.

"librandom/librandom_names",
"nestkernel/nest_names",
"topology/topology_names",
]

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

I don't think hard-coding the names is a good idea. Why not

name_files = [n[:-2] for n in glob.glob('*/*_names.h')]

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

Isn't that a bit dangerous? I can imagine other files to also end in _names.h without actually defining Names in the sense that the checker should check for. The current list of files was stable over quite some years and I assume that to be the case for some more...

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

OK.

for line in lines:
if "extern const Name" in line:
names_header.append(line.split("Name ")[1].split(";")[0])
names_header.sort()

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

Could you turn this into a function or loop to avoid code duplication? I also wonder if it would not be safer and more efficient to test with a regular expression, especially to avoid a that a single extra space confuses the test. Something like extern\s+const\s+Name\s+([_\w]+)\s*; and then get the name from the match group.

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

I've turned it into a function using regex matching in 45a67f7.

"topology/topology_names",
]

names_defined = set()

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

I would make this a list. Then you can test later if names_defined == unique(names_defined). This catches cases where automated merges duplicate lines---has happened a few times.

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

I've refactored this together with the check for unused names in 5a58f65 and think that catching errors with automatic merges (and other unrelated things) should not be covered by this test if it does not come for free.

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

Ok.

names_defined.sort()


from subprocess import check_output

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

Please collect all imports at the start of the file.

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

Done in ac03064.


from subprocess import check_output

grep_cmd = ["grep", "-ro", "names::[A-Za-Z0-9_]*", source_dir]

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

Would you add a comment pointing out why you use Unix grep here instead of Python?

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

Done in ac03064.

names_used = list(names_used)
names_used.sort()

for d, u in zip(names_defined, names_used):

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

I think you could do this more efficiently as set(names_defined) - set(names_used).

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

I've refactored the test logic for unused Names in 5a58f65.

* efficient and consistent. Creating a Name from a std::string is in
* O(log n), for n the number of Names already created. Using
* predefined names makes data exchange much more efficient as it
* uses integer comparisons instead of string comparisons internally.
*/

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

Please extend this comment to explain the rules of ordering in this file.

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

I've extended the comment in 50b99d1.

const Name I_NaP( "I_NaP" );
const Name I_stc( "I_stc" );
const Name I_std( "I_std" );
const Name I_syn( "I_syn" );

This comment has been minimized.

@heplesser

heplesser Mar 14, 2018
Contributor

Please keep I_syn, #812 will use it.

This comment has been minimized.

@jougs

jougs Mar 14, 2018
Author Contributor

I've re-added it in f6bbc93.

@heplesser heplesser requested a review from terhorstd Mar 14, 2018
@heplesser
Copy link
Contributor

@heplesser heplesser commented Mar 19, 2018

The Open NEST Developer Meeting on 19 March has no objections to removing the comments from the Names files.

@heplesser heplesser requested a review from stinebuu Mar 19, 2018
@heplesser heplesser removed the request for review from terhorstd Mar 19, 2018
Copy link
Contributor

@stinebuu stinebuu left a comment

Excellent work! I don't have any complaints.

@heplesser heplesser merged commit 6ccfa02 into nest:master Mar 19, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.