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 absolute includes in MO module #45

Closed
wants to merge 41 commits into from
Closed

Fix absolute includes in MO module #45

wants to merge 41 commits into from

Conversation

ropinho
Copy link
Contributor

@ropinho ropinho commented Aug 22, 2019

All files in MO directory was updated.

The include directives was changed from absolute to relative format.
EO files inclusions in absolute format. It supposed which the Eo module is installed before than Mo module.

the EO module has also been updated. See #43 .

I tested the installation and usage after these changes and apparently is all right. The changes is not affecting the classes usability.

@nojhan
Copy link
Owner

nojhan commented Dec 6, 2019

Thanks for the proposal. However, I unfortunately don't think it's feasible this way, because modules that depends on EO have an /src/ subdir, which comes in the way of relative paths.
We should rethink the organization of the directories before being able to do that.

For the time being, I will not do this kind of refactoring in MO without its maintener approval and a refactoring of subdirectories.

Also, those commits have a lot of conflicts with PR#43.
Things to pay attention to:

  • do not replace <> for STL modules.
  • use an unicode-friendly replace method (we have non-ascii names in the comments)

To make a PR that is easy to check and merge for the maintener, forge atomic commit. At least, rebase the branch with 'git rebase -i master' and squash all commits in a single one, so that diffs would be easy to review.

@nojhan nojhan closed this Dec 6, 2019
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.

None yet

2 participants