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

New names for files, classes in manuscripts2 #72

Closed
jgbarah opened this issue Jul 10, 2018 · 2 comments · Fixed by #78
Closed

New names for files, classes in manuscripts2 #72

jgbarah opened this issue Jul 10, 2018 · 2 comments · Fixed by #78

Comments

@jgbarah
Copy link
Contributor

jgbarah commented Jul 10, 2018

Even when the structure in manuscripts2 starts to take shape, we need to discuss better names for the structures there. For example, "new_functions" is not a good name ;-)

Could we discuss here how to name files, classes and methods?

@jgbarah
Copy link
Contributor Author

jgbarah commented Jul 18, 2018

Since I see no movement here, let me start with a proposal.

Files:

  • new_functions.py to elasticsearch.py (since it deals with queries to ES).
  • derived_classes.py, for now, could be in elasticsearch.py to, since it is very little code, and not really different.
  • test_new_functions.py could move to tests/test_elasticsearch.py (that is, in the tests directory of the repo), so that we have tests integrated, and working.
  • Readme.md could be README.md

In addition, we would need to modify setup.py so that manuscripts2 is included in the package, and can thus be installed and tested automatically.

For now, we can leave the names of the classes and methods as they are. We could revisit that later...

What do you think, @aswanipranjal?

@aswanipranjal
Copy link
Contributor

I am sorry, I couldn't think of any appropriate names. Thanks for this proposal! These are totally fine with me. I'll make the changes and create a PR.

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 a pull request may close this issue.

2 participants