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

Change repo structure. Stage 2 #192

Merged
merged 14 commits into from
Jun 29, 2018
Merged

Change repo structure. Stage 2 #192

merged 14 commits into from
Jun 29, 2018

Conversation

StrikerRUS
Copy link
Member

This PR is a continuation of #191.

Refer to #166.

RGF software package has been moved into a separate 1st level folder.

Also, instructions about how to compile RGF have been moved from python-package's Readme.rst to RGF's own README.md.

In addition, missed list item about MinGW fix in RGF changelog has been added,


def copy_files_helper(folder_name):
src = os.path.join(CURRENT_DIR, os.path.pardir, folder_name)
if os.path.exists(src):
Copy link
Member

Choose a reason for hiding this comment

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

Is os.path.isdir better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just out of curiosity, why is it better?

Copy link
Member

@fukatani fukatani left a comment

Choose a reason for hiding this comment

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

LGTM excepts above the comments.


if not os.path.exists(os.path.join(CURRENT_DIR, '_IS_SOURCE_PACKAGE')):
copy_files_helper('RGF')
# copy_files_helper('FastRGF')
Copy link
Member

Choose a reason for hiding this comment

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

# copy_files_helper('FastRGF')

Copy link
Member Author

Choose a reason for hiding this comment

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

It's for the next stage. Do you want I remove this?

Copy link
Member

Choose a reason for hiding this comment

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

position of "#" may be strange, I mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll just remove it.

else:
raise Exception('Cannot copy {} folder'.format(src))

if not os.path.exists(os.path.join(CURRENT_DIR, '_IS_SOURCE_PACKAGE')):
Copy link
Member

Choose a reason for hiding this comment

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

Is os.path.isdir better?

@riejohnson
Copy link
Member

Regarding README, maybe "existing makefile" -> "the provided makefile" in 3.1.3. and 3.2.1? (I'm assuming that "existing makefile" means "makefile that is provided by this software package". Right?) I also found "Existing" in the section titles a bit hard to understand, and so how about "3.1.2. Visual Studio (using the provided solution file)", "3.1.3. MinGW (using the provided makefile)", and "3.2.1. g++ (using the provided makefile)? Or maybe, "3.1.2. Visual Studio (solution provided)", 3.1.3. MinGW (makefile provided)" and so on? This is just a minor suggestion, and I'll leave it to you. Thanks.

@StrikerRUS
Copy link
Member Author

@riejohnson Thank you for your suggestion. Yeah, "provided" seems to be more appropriate than "existing".

Copy link
Member

@fukatani fukatani left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your contribution!

@StrikerRUS StrikerRUS merged commit aad3fd3 into master Jun 29, 2018
@StrikerRUS StrikerRUS deleted the repo_structure branch June 29, 2018 17:54
This was referenced Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants