Skip to content

Conversation

@aumghelani
Copy link

No description provided.

@parthckaria
Copy link

Please make PR against the dev branch.

@AjinkyaDahale
Copy link
Collaborator

Needs plenty of changes. Some of these may be delayable until later, but ideally do them first:

  1. Make PR against dev as @parthckaria mentioned. Do this immediately so that we can review and test with the latest changes, and catch conflicting changes faster.
  2. Keep the number of commits limited. It is fine to keep them separate while working, but you must squash them into meaningful chunks before the final merge.
  3. Avoid merge commits in a PR branch. Rebase instead. This can be done with 2 above.
  4. Follow proper git commit message conventions (this link may be helpful).

osdag = "osdag.osdagMainPage:do_stuff"

[project.scripts]
osdag-cli = "osdag_cli:testing"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep osdag_cli under the osdag module (so it should read osdag.osdag_cli:testing). Also, change name of the method to something less temporary. Can main do the job?

Comment on lines +12 to +13
# Reads a plain text file line by line, extracts the 'Module' field,
# and determines the module type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Reads a plain text file line by line, extracts the 'Module' field,
# and determines the module type.
# Given an Osdag input file (.osi), extracts the 'Module' field,
# and determines the module type.

Comment on lines +16 to +23
# with open(file_path, 'r') as file:
# for line in file:
# # Check if the line contains the 'Module' field
# if line.startswith("Module:"):
# # Extract and print the module name
# module_name = line.split(':', 1)[1].strip()
# click.echo(f"Module: {module_name}")
# return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recall in a recent meeting you showed this code as working. Have you not updated the changes here?

Comment on lines 13 to 15
'console_scripts': [
'osdag-cli = osdag_cli:determine_module',
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may get us too many entry points. OK for testing, but eventually we want something like:

osdag-cli --determine-module <filename>

@@ -0,0 +1,17 @@
# setup.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're already using pyproject.toml, which is a more modern method. Probably safe to delete.

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