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

Remove unconventional location for header files #54

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

robertoaloi
Copy link
Contributor

@robertoaloi robertoaloi commented Mar 1, 2022

This PR is an alternative approach to #49.

The rationale for this solution is that moving the file, instead of aliasing it, is a much simpler approach and it makes the code base use a more conventional inclusion pattern. While having sub-directories within include and including headers with `-include('my_dir/my_header.hrl') technically works in OTP, it's a non conventional choice that makes it harder to include the file from external applications. Tools such as Erlang LS may require additional tweaking by the user to get code navigation and other features working with the code base, too.

Breaking backward compatibility is probably not a big deal in this case, given that the header file was explicitly marked as internal in the previous implementation. Making the move explicit would also allow users to immediately spot (and correct) the difference at compile time and take the opportunity to cleanup extra paths and directories in their configuration which were only present due to the current setup. So, it feels like a step in the right direction.

@robertoaloi robertoaloi closed this Mar 1, 2022
@robertoaloi
Copy link
Contributor Author

robertoaloi commented Mar 1, 2022

I opened this PR immediately before realizing you already have #49 as an alternative solution. Let me check if that works for our use case...

@robertoaloi
Copy link
Contributor Author

@dumbbell Any opinion on this?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.564% when pulling 9a9f7a2 on erlang-ls:avoid-internal-includes into 39981b9 on yakaz:master.

@dumbbell
Copy link
Collaborator

dumbbell commented Mar 4, 2022

I agree with you, the breakage should be ok.

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