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

Namespace and organize lcf headers #342

Closed
2 tasks done
fmatthew5876 opened this issue Sep 7, 2019 · 6 comments · Fixed by #361
Closed
2 tasks done

Namespace and organize lcf headers #342

fmatthew5876 opened this issue Sep 7, 2019 · 6 comments · Fixed by #361

Comments

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 7, 2019

We need to reorganize the code before we can declare the liblcf API stable.

  • Make all users #include <liblcf/foo.H> to avoid header conflicts.
  • Put all liblcf code in an lcf:: namespace.
@Ghabry
Copy link
Member

Ghabry commented Sep 7, 2019

  • Get rid of everything in "Data::", global variables are ugly

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Sep 7, 2019

As a first step, we could move Data:: to player and get it out of liblcf.

There are some parts of the liblcf parser that depends on grabbing stuff from Data::. That has to be refactored.

@fmatthew5876 fmatthew5876 changed the title Namespace and orgranize lcf headers Namespace and organize lcf headers Dec 21, 2019
@fmatthew5876
Copy link
Contributor Author

I'd like to propose we do this header file and namespace change sometime when we cut 6.2 release.

The main thing blocking me from doing it now is rebase hell. I wouldn't want to make a namespace PR here and in Player and have it stay for a long time and cause lots of rebase conflicts with other PRs.

Seems like right before a release when we have a rush to get everything in would be a good time to cut over.

@Ghabry Ghabry added this to the 0.7.0 milestone Dec 21, 2019
@Ghabry
Copy link
Member

Ghabry commented Dec 21, 2019

moving this to 0.7.0 for now

@carstene1ns
Copy link
Member

Seems like right before a release when we have a rush to get everything in would be a good time to cut over.

Yeah, because nobody makes mistakes when in a rush. 🙄 This is to be done right after a release, not before.

@fmatthew5876
Copy link
Contributor Author

Ok sure, but maybe we could try to coordinate it, like right after the 6.2 release so it doesn't stay pending too long? This one is a rebase nightmare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants