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

Brushed up the source code #2

Merged
merged 9 commits into from
Oct 26, 2021
Merged

Brushed up the source code #2

merged 9 commits into from
Oct 26, 2021

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Sep 7, 2021

Maintainer can now python setup.py bdist_wheel to make a functional wheel.

* Moved to `setuptools`, away from deprecated `distutils`.
* Removed unconventional packaging of an examples folder.
* Added install requirements.
* Moved stuff to root folder instead of `extras` folder
* Removed `package_dir` from setup
* Fixed `matplotlib.font_manager` import error
@jougs
Copy link
Contributor

jougs commented Sep 7, 2021

Thanks for this contribution!

Can you please quickly comment in how far this complements of conflicts with #1? At least the move of files seems to be incompatible.

@Helveg
Copy link
Contributor Author

Helveg commented Sep 7, 2021

It seems that #1 places the files in the root folder, I wouldn't do that. It's more conventional for Python projects to gather their source code in a folder named after the package (i.e. nest_connplotter if the package name is to be nest-connplotter as in #1). This will simplify all packaging and other convention-sensitive operations

@jougs
Copy link
Contributor

jougs commented Sep 7, 2021

I commented the same thing in #1 (i.e., files in sub-directories). Does this PR then replace #1?

@Helveg
Copy link
Contributor Author

Helveg commented Sep 7, 2021

@jougs I added the missing changes from that PR here and now it can replace it

Copy link
Collaborator

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

@Helveg thanks for these changes! Isn't it better to have the package name as nest-connplotter instead of ConnPlotter?

@Helveg
Copy link
Contributor Author

Helveg commented Sep 7, 2021

Both names are available on PyPI. Using nest-connplotter would mean that we have a different module name than package name with nest_connplotter as its closest match. I don't know if anyone else has any suggestions? nestplotlib? 😂 I believe it is PEP8 that recommends module names to be underscore cased, but it's in one of the PEPs for sure. I left it at ConnPlotter as I was impartial and wanted the least amount of refactoring work ;)

@jougs
Copy link
Contributor

jougs commented Sep 7, 2021

I have no strong opinions either way, but would like things to be somewhat consistent. Here's the current situation:

Repository PyPI package Module
nest-simulator nest-simulator nest
ode-toolbox odetoolbox odetoolbox
nestml nestml nestml
nest-sionlib-reader nestio
nest-connplotter
nest-desktop* nest-desktop?
nest-client* nest-client?

* soon in a NEST Initiative GitHub account near you.

@heplesser, @terhorstd: maybe we should discuss this in the next open VC?

@jougs
Copy link
Contributor

jougs commented Sep 7, 2021

@Helveg: would you please be so kind and amend 6de4e39 to add @pnbabu as co-author? Thanks!

@Helveg
Copy link
Contributor Author

Helveg commented Sep 7, 2021

I can amend the commit message, is there a format I should follow, could you perhaps give me the exact string to amend?

@jougs
Copy link
Contributor

jougs commented Sep 7, 2021

Adding Co-authored-by: Pooja Babu <[email protected]> on a separate line should do the trick.

@heplesser
Copy link
Contributor

@jougs @terhorstd Putting the naming onto the agenda of the nest Open VC is a good idea.

@terhorstd
Copy link

👍 Added as discussion point to the next meeting.

Co-authored-by: Pooja Babu <[email protected]>
@jougs
Copy link
Contributor

jougs commented Sep 13, 2021

The naming discussion in the Open NEST video conference today resulted in the creation of nest/nest-simulator#2161 and the decision to rename this repository from nest-connplotter to connplotter. All further naming and consistency discussion should continue there.

These steps should now allow to also name the PyPI package and the module the same way as the repository. @Helveg: could you please update this PR accordingly? Thanks!

@Helveg Helveg requested a review from pnbabu September 13, 2021 18:00
Copy link
Collaborator

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion.

@jougs
Copy link
Contributor

jougs commented Sep 17, 2021

When merging this one, nest/nest-simulator#2147 should also be merged.

Co-authored-by: Jochen Martin Eppler <[email protected]>
@jougs jougs merged commit 12e0956 into nest:main Oct 26, 2021
@Helveg Helveg deleted the refactor branch October 27, 2021 09:08
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.

5 participants