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

Tydying python code based on issues found by linter #57

Merged
merged 16 commits into from
May 20, 2024
Merged

Conversation

julianstirling
Copy link
Collaborator

PyLint has detected a number of issues which are now checked as GitHub Actions.

The plan is to get the PyLint rating above 9.75 if possible. No plan to aim for fixing every issue. For example some warnings about high numbers of of arguments or instance attributes are easier to ignore when passing around data.

@julianstirling julianstirling requested a review from jmwright May 16, 2024 02:13
@julianstirling julianstirling marked this pull request as ready for review May 16, 2024 02:13
@julianstirling
Copy link
Collaborator Author

Hi @jmwright, it is now 3am and I have been working on this for 17 hours straight. The code is far from perfect but it is now much more modular and parametric, with a few more doc_strings and comments. But they are still pretty bare bones.

ShelfBuilder is somewhat de-tangled to remove the huge number of instance attributes, some defined well after initialisation. These are now generally derived properties. I also force the main properties to be shared at initialisation and build the front on initialisation. This is needed because otherwise if you ran other methods before the front is created you will get errors.

I have removed the old nimble_tray.py and instead there is a generic tray in tray_6in.py I have nominally updated the server. But not in a way that is tested (I left a note about this as some the variable to pass have now changed.)

There is still a fair bit of work to do to properly parametrise the code base. I have captured some of that in this issue. There is probably more, but I now need to sleep!!

@jmwright
Copy link
Contributor

@julianstirling Thanks for all the work on this. It is a lot to take in, but I did my best to review it.

I have peppered suggestion comments throughout. Many of these are related to minor things in docstrings. We do not need to burn time discussing these, just mark as resolved without merging the suggestion if you don't want it.

I also made a few comments on higher level questions with the code and design.

When I run python generate.py (on this branch) per the instructions, I get the following error.

Starting build
Generating components
INFO:exsource:Processing export: rack_leg
Traceback (most recent call last):
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/exporters.py", line 164, in run_executable
    ret = subprocess.run(args, check=True, capture_output=True)
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/subprocess.py", line 526, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['cq-cli', '--params', 'length:116;hole_spacing:14;', '--infile', '../mechanical/components/cadquery/rack_leg.py', '--outfile', './printed_components/beam.step']' returned non-zero exit status 100.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/jwright/repos/nimble/generate.py", line 38, in <module>
    generate_example_configuration()
  File "/home/jwright/repos/nimble/generate.py", line 35, in generate_example_configuration
    generate(selected_devices_ids)
  File "/home/jwright/repos/nimble/generate.py", line 25, in generate
    runner.generate_components(config.components)
  File "/home/jwright/repos/nimble/nimble_orchestration/orchestration.py", line 47, in generate_components
    self._run_exsource(exsource_path)
  File "/home/jwright/repos/nimble/nimble_orchestration/orchestration.py", line 95, in _run_exsource
    processor.make()
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/tools.py", line 98, in make
    unprocessed_exports = self._process_exports(unprocessed_exports)
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/tools.py", line 135, in _process_exports
    cqe.process_export(output_file_statuses=self._all_output_files)
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/exporters.py", line 186, in process_export
    self.run_executable(output, require_x)
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/exporters.py", line 169, in run_executable
    raise RuntimeError(f"\n\n{self.name} failed create file: {output.filepath}"
RuntimeError: 

CadQuery failed create file: ./printed_components/beam.step with error:


Traceback (most recent call last):
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/cq_cli/main.py", line 38, in build_and_parse
    raise (build_result.exception)
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/cadquery/cqgi.py", line 105, in build
    self.set_param_values(build_parameters)
  File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/cadquery/cqgi.py", line 137, in set_param_values
    raise InvalidParameterError(
cadquery.cqgi.InvalidParameterError: Cannot set value 'hole_spacing': not a parameter of the model.

@julianstirling
Copy link
Collaborator Author

Thanks @jmwright I'll get your changes merged in and check to see if I can replicate that error. Will probably be Monday as I'm on the road this weekend.

@julianstirling
Copy link
Collaborator Author

@jmwright The generate.py script should work again. Thanks for catching this. I did run it periodically, but I clearly didn't run it right at the end. We should get that in the CI soon, so we catch these kind of things.

I think I have merged all of your suggestions. For every high level comment that needed an action I have either opened an issue or added it as a task to an issue.

Assuming the code works are you happy for this to be merged? Feel free to merge it if you accept the review

Thanks again

@jmwright
Copy link
Contributor

Thanks @julianstirling !

@jmwright jmwright merged commit 2f5e5bd into master May 20, 2024
4 checks passed
@jmwright jmwright deleted the python-tidy branch December 27, 2024 14:48
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