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

Some recommended style issues #6

Closed
ocaisa opened this issue May 10, 2019 · 3 comments
Closed

Some recommended style issues #6

ocaisa opened this issue May 10, 2019 · 3 comments

Comments

@ocaisa
Copy link
Contributor

ocaisa commented May 10, 2019

@dwhswenson raised some style issues when reviewing the code:

there are a lot of kwarg updates that might be cleaned up with a function like

# instead of making a dictionary to update with all at once, call update_kwargs for each name
def update_kwargs(kwargs, obj, name, default):
    kwargs.update({name: getattr(obj, name, getattr(kwargs, name, default))})

The idiom with the nested getattrs is repeated several times, and requires the use of name 3 times. It is also the source of some of the longer lines (not to mention some harder-to-read and somewhat inconsistent formatting from black).

@dwhswenson
Copy link
Collaborator

dwhswenson commented May 10, 2019

To clarify, the sorts of things that I thought were inconsistent in black's formatting are lines like these:

"mpi_launcher": getattr(
cluster, "mpi_launcher", getattr(kwargs, "mpi_launcher", MPIEXEC)
),
"mpi_tasks": getattr(
cluster,
"mpi_tasks",
getattr(kwargs, "mpi_tasks", self.default_mpi_tasks),
),

In the first dict entry, all the arguments to getattr are on one line. In the second entry, each argument has its own line. It takes a second to see that these are all doing essentially the same thing; putting the behavior in a function would make it more readable, in my opinion. Also, easier to write: only need to include the string for the name once, instead of 3 times.

@AdamWlodarczyk
Copy link
Collaborator

Agree. String literals should be introduced once.

@ocaisa
Copy link
Contributor Author

ocaisa commented May 29, 2019

This is also resolved in #9

@ocaisa ocaisa closed this as completed May 29, 2019
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

No branches or pull requests

3 participants