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

Make merging PRs and updating IDs easier with a node script that fixes duplicate IDs and orders them in the JSON data #11

Closed
EddyVinck opened this issue Oct 20, 2019 · 9 comments
Labels
critical This issue has a high-priority enhancement New feature or request help wanted Extra attention is needed

Comments

@EddyVinck
Copy link
Owner

EddyVinck commented Oct 20, 2019

In the data directory you will find a few files with relational data. Since this repository depends on pull requests to add more data and the fact that multiple pull requests need to be merged at a time is a problem that needs to be solved.

A few pull requests in and I'm already feeling the problem with the merge conflicts. If there was a node script that would fix duplicate IDs, auto-increment them and order them in a logical way that would really help people contribute and save everyone a lot of time.

@EddyVinck EddyVinck added hacktoberfest help wanted Extra attention is needed labels Oct 20, 2019
@EddyVinck EddyVinck changed the title Make updating IDs easier Make merging PRs and updating IDs easier with a node script that fixes duplicate IDs and orders them Oct 20, 2019
@EddyVinck EddyVinck changed the title Make merging PRs and updating IDs easier with a node script that fixes duplicate IDs and orders them Make merging PRs and updating IDs easier with a node script that fixes duplicate IDs and orders them in the JSON data Oct 20, 2019
@taksuparth
Copy link

Hi @EddyVinck
This seems an interesting problem, I would like to take it up. Currently, I am not sure how can I fix it. Let me do some digging.

@EddyVinck
Copy link
Owner Author

Sure @taksuparth go ahead! You can message me here or on Slack if you have any questions.

@dios-david
Copy link
Contributor

dios-david commented Oct 20, 2019

As discussed on Slack, I'm building a proof of concept which:

Replaces the auto-increment IDs with random generated ones
This solution will provide easier contribution, as people won't need to care about rewriting their newly added IDs.

Adding a simple CLI tool which helps the contributor to add new content to the DB
People won't need to edit raw JSON files to contribute with new contents. The tool itself will handle the ID bindings between entities (e.g. talks and speakers). In the future it will be nice to have a web interface to manage this process.

@dios-david
Copy link
Contributor

dios-david commented Oct 20, 2019

Check my initial implementation here: #12
Any feedback is welcomed! :)

@EddyVinck
Copy link
Owner Author

@dios-david I just tried it out, and like I already said on Slack the code is organized really well! I don't really have much experience from coding CLIs but I definitely learned a thing or two just by looking at the code. Nice job 👍

The CLI itself is pretty good for the amount of time you put into it so far. This lowdb library you used seems pretty handy for a project like this.

I could really see this turning out as a great foundation for a GUI version for the website like you suggested. I don't know if you've ever used something like Docusaurus (we use this for https://falcon.deity.io), but they have this nice button that forks the original repo and creates a new branch for you to work on. If we could automate that combined with a GUI editor that merges in the new data with the current data I think we would have a great solution.
We could save the data someone is going to add in their localstorage for a bit in case they accidentally close the tab and then wipe their localstorage again when they're done (or something like that).

What I'm currently not seeing is a way to make working with merge conflicts easier.
Right now when two people fork the master branch and make a PR the second PR always needs to merge the new master branch (obviously) and then there are merge conflicts. Ideally there would be a CLI option to merge the current data on master with the data from the second PR.

The challenge here is that the entire database is now 1 file (db.json) and the merge conflict could add a new conference, multiple speakers, some categories and of course the talks. I don't think git is smart enough to keep all that data in their respective array. Maybe adding a __typename property (like Apollo does with GraphQL in Apollo Client) to each object would make the job of rearranging the file easier.

Another challenge I see to solve this problem is that it's not really possible to differentiate the new data in the PR from the data in the master branch. Maybe analyzing the characters added in merge conflicts could solve this, but I'm not sure if that is a good approach. Another option could be to add a added_at timestamp to every piece of data and comparing those.

Some quality of life improvements could be:

  • Adding a search option to the questions about speakers, categories, conferences
  • Adding an option to create a new speaker, category or conference on the fly when adding a talk

Honestly, great work so far. I'm really interested to see where we can take this 🙂

This was referenced Oct 22, 2019
@jglover
Copy link
Contributor

jglover commented Oct 25, 2019

This has some level of crossover with the issue I just raised. I'd be interested in your thoughts #43

@EddyVinck
Copy link
Owner Author

@jglover I like the suggestion! Although I'm not sure if this is the right approach since we're already looking into a JSON "database" in the issue you referenced.

We could add interfaces like you suggested, while still keeping the data in a JSON file managed by lowdb. I think the way the data is accessed is the thing that is most likely to go wrong. For example: using talk.title as opposed to talk.main_title.

You raise valid concerns about the data, like maintaining data integrity, merge conflicts and refactoring workloads which I hope to tackle with tests, scripts and perhaps even a bot on GitHub after we get this JSON database approach working the way we want. The tests can be run with Travis CI, which is free for open source projects. The bot could update and auto-merge the latest master branch into PRs.

When the CLI is working I hope we can take that to the next level in the form of a GUI on the website to provide a simple and accessible way of adding data.

My main concern with swapping our JSON approach with a TypeScript approach, is it being accessible to many developers. A lot of people from different development backgrounds understand JSON, while only a fraction will understand TypeScript.

@EddyVinck
Copy link
Owner Author

EddyVinck commented Oct 26, 2019

@dios-david I hope to take a look at your PR again later today after fixing this bug: #45

If not today then probably tomorrow.

As for the CLI, I'd like to explore adding an option that fixes a merge conflict. If I have time I might also look into options like searching for a speaker name of category. I'll probably make a PR to your branch. Writing a CLI is not something I'm very experienced with so this should be a fun learning experience 🙂

@EddyVinck EddyVinck pinned this issue Nov 6, 2019
@EddyVinck EddyVinck added enhancement New feature or request critical This issue has a high-priority and removed hacktoberfest labels Nov 6, 2019
@EddyVinck
Copy link
Owner Author

EddyVinck commented Nov 6, 2019

I just created a new label "critical" for issues with a high priority. I'd like to prioritize this feature over other features for now.

Life if pretty busy in and outside of work right now, so thank you for your patience. I'll start working on this when I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical This issue has a high-priority enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants