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

Fix typos, add venv to .gitignore #6

Open
wants to merge 3 commits into
base: stable
Choose a base branch
from

Conversation

kinow
Copy link

@kinow kinow commented Jul 5, 2022


name: Pull Request template
about: Fix typo, use the platform module for OS detection
title: ''
labels: ''
assignees: ''


Status

READY

Description

Hi, this PR does not fix any existing issue. I cloned the project, imported the Java project in Eclipse, and the Python binding in PyCharm. PyCharm found a few issues, and I also couldn't install it with pip install -e . (now it's failing with another error, but no Python traceback).

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Project impact

List the general project components that this PR will affect:

  • pycompss

Related PRs

How Has This Been Tested?

  • I have added a new test with number:
  • I have modified a test to check this. Test number:
  • I have tested it manually in a local environment.
  • I have tested it manually in a supercomputer.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • I have run the script to add headers if new files are present (i.e. utils/scripts/header_setup/replace_all.sh)
  • I have rebased my branch before trying to merge.

Copy link
Author

@kinow kinow left a comment

Choose a reason for hiding this comment

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

First PR to compss. Haven't learned how to run the tests, so didn't check that option in the pull request template. Hoping CI will find any issues with this PR 👍

@@ -49,7 +50,7 @@
"-fstack-protector",
]

TARGET_OS = os.environ["TARGET_OS"]
TARGET_OS = platform.system()
Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure if there was a reason for using this environment variable, but if it's legacy code, then maybe the platform module is a more suitable option? The TARGET_OS is not defined in my env.

@kinow kinow changed the title Suggested changed Fix typo, use the platform module for OS detection Sep 9, 2022
@kinow kinow changed the title Fix typo, use the platform module for OS detection Fix typos, add venv to .gitignore Sep 9, 2022
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.

1 participant