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

Repository refactoring #1210

Closed
gempain opened this issue Oct 26, 2017 · 8 comments
Closed

Repository refactoring #1210

gempain opened this issue Oct 26, 2017 · 8 comments
Labels
developer-experience Dev tooling, test framework, and CI

Comments

@gempain
Copy link

gempain commented Oct 26, 2017

Hello,

I discovered this repository today and made some local development. It's fun to use, it works well, and it's awesome.

However, I have a feeling that it is going to become unmaintainable as people keep adding badges in the server.js file. I feel like the directory structure would benefit a good refactoring. I haven't looked at all the code in there, so cannot really judge it's quality.

In the first place I would:

  • create a badge/ directory and move all the tags in server.js to seperate files in there
  • migrate code to Typescript, create classes, utility classes... and use a bundler like Webpack
  • document all utility functions like getBadge()
  • try to refactor the directory structure to make sure it's properly organized and documented

This is just a suggestion, I feel like it would help developers contribute much better to this really fun project.

All the best;

@paulmelnikow paulmelnikow added the developer-experience Dev tooling, test framework, and CI label Oct 26, 2017
@paulmelnikow
Copy link
Member

Hi! I'm glad you’re having fun hacking on Shields. And thanks for your suggestions! I agree there is a lot of potential to make it friendlier to contributors, and I’m really committed to that goal. Since this project tries to integrate everyone’s developer tools, it’s particularly conducive to having a wide community of contributors. That means we should aim for a high level of friendliness in the code base. Things have improved a lot in the last several months, though there is ever more to do.

At this point it's a given that the services will not live in server.js. @Daniel15 is working on a new function for registering services (#963) and it wouldn’t hurt to leave comments there if you’re interested in how that shapes up.

When that’s done I’m anticipating the services will be straight-out rewritten, with tests written for the services which don’t have them already. I’m not sure exactly how many badges we have, though there are way too many to do at once. So this will be gradual.

In some cases the helper functions will just go away. For example getBadge, getLabel won’t be needed anymore. Documenting and unit testing the utility functions which need to be kept, such as the formatting functions, will be really helpful in facilitating that rewrite and distributing it to many hands. Also necessary are tests for services that don’t have them yet. Basically service tests are half the work of rewriting a badge, so having them on hand will accelerate the rewrite by 2x.

Another item you mention is using a bundler. Modernizing the front end would be great, particularly with an eye to requests like #466 and #701 which would dramatically improve the experience. I don’t mind if we tackle the tooling before functionality. I got the ball rolling in #1163 which dynamically generates the badge listing.

If you’d be interested in taking on any part of this, I’d be happy to give feedback and direction!

@gempain
Copy link
Author

gempain commented Oct 27, 2017

Hello @paulmelnikow,

Great reading you, and I am glad my suggestions are being taken with such humbleness :) I am always afraid when making this kind of suggestions as people tend to get irritated or upset.

I have taken a look at the threads you mentioned. I would be happy to participate to the discussions. When I have time, I will try to come up with a Typescript + bundler setup, and refactor a bit services already (especially splitting them in separate files to lighten the main server file.

I also feel like it would be quite important to separate core code from front-end code. Front-end is one thing, and can be thought way differently than backend. Offering users a great UI with a test playground is great, but it must be thought (IMHO) separately, without really thinking (in the first place to the core code).

I'll fork the project and come up with something, then come back here and let you guys pick all or part of what I have changed :)

Thanks again for your great reply !

@paulmelnikow
Copy link
Member

Just wanted to encourage you to talk about what you're planning before investing too much in the implementation! Also because small PRs are much easier to respond to than big PRs which move a lot of code.

My suggestions would be:

  1. Document and unit test the utility functions which need to be kept, such as the formatting functions (to expedite service rewrite).
  2. Write tests for services which don't have them yet (to expedite service rewrite).
  3. Set up a bundler for the front end code.

Let's hold off on Typescript. Please don't refactor the services. After #963 we're going to rewrite them.

@gempain
Copy link
Author

gempain commented Nov 1, 2017

No worries, I wouldn't go into refactoring the services, there is way too much work for myself only.

@paulmelnikow
Copy link
Member

Let me know if you start on the frontend work!

@FezVrasta
Copy link
Contributor

I don't think TypeScript is a good idea, lot of people don't use it and wouldn't be comfortable in contribute to the project

@paulmelnikow
Copy link
Member

I opened a WIP PR for the new frontend: #1273.

@paulmelnikow
Copy link
Member

Discussion of Flow / TypeScript is in #1272.

The server rewrite is here: https://github.com/badges/shields/projects/2

Feel free to reopen if you'd like to discuss further!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

No branches or pull requests

3 participants