Skip to content

Added run command in makefile - #70

Closed
delterr wants to merge 3 commits into
ONSdigital:mainfrom
delterr:makefile_run
Closed

Added run command in makefile#70
delterr wants to merge 3 commits into
ONSdigital:mainfrom
delterr:makefile_run

Conversation

@delterr

@delterr delterr commented Sep 12, 2024

Copy link
Copy Markdown

Addresses #69

@delterr

delterr commented Sep 12, 2024

Copy link
Copy Markdown
Author

not ready to merge yet as i had set the command to specifically run to ons_python but need to generalise to all module names with jinja

Comment thread Makefile
Comment on lines +41 to +43
.PHONY: run
run: ## Run the application
poetry run python {{module_name}}

@MebinAbraham MebinAbraham Oct 25, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

  1. This is the wrong file; you need to add this to https://github.com/ONSdigital/ons-python-template/blob/main/project_template/Makefile.jinja (templates makefile, not this repo's root template)

  2. You also need to make it agnostic to the package manager, so it should be:

{{ package_manager }} run python {{ module_name }}
  1. Please add this new __main__.py file to https://github.com/ONSdigital/ons-python-template#structure

Comment on lines +1 to +8
from calculator import Calculator
calc = Calculator()

calc.add(5)

calc.subtract(2)

print(calc.cumulative_total)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would currently raise lint failures.

Suggested change
from calculator import Calculator
calc = Calculator()
calc.add(5)
calc.subtract(2)
print(calc.cumulative_total)
from calculator import Calculator
calc = Calculator()
calc.add(5)
calc.subtract(2)
print(calc.cumulative_total)

@sanjeevz3009

Copy link
Copy Markdown
Collaborator

@delterr If you address the above comments, we can progress this

@sanjeevz3009

Copy link
Copy Markdown
Collaborator

Closing the PR due to inacivity and the following PR superseeding the current one: #114.

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.

3 participants