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

Lots of formatting fixes #37

Merged
merged 24 commits into from
Feb 14, 2024
Merged

Lots of formatting fixes #37

merged 24 commits into from
Feb 14, 2024

Conversation

jeffclay
Copy link
Contributor

I started with an attempt to resolve issue #25 and being brand new to RST and Sphinx, etc. this was... interesting.

I don't think I broke anything. I fixed all the errors that my changes generated. All in all, I think it turned out well considering the scope of the changes but I'll let the experts in documentation be the judge.

Most of the changes were made by a Python script so that I could iterate, investigate and adjust things across all the documents like applying a template. I put the exact script that is responsible for the changes on my Github.

Just like the commits, the script is fairly straightforward, at the bottom main.py in the process_directory function you'll see a list of functions executed sequentially responsible for each commit of this PR. I did it this way trying to make all the changes more manageable and comprehensible.

Copy link

github-actions bot commented Jan 29, 2024

🪓 PR closed, deleted preview at https://github.com/freeipa/freeipa.github.io/tree/gh-pages/pull/37/

@antoniotorresm
Copy link
Collaborator

This is an amazing work, thanks a lot!
Could you squash your commits into one (add all the titles to the single commit's message), and fix the merge conflict?

@jeffclay
Copy link
Contributor Author

Does Github not have the option to squash commits when merging? I'm more familiar with Gitlab and figured that the multiple change specific commits would be better for change tracking and for anyone who tries to review the 11k/9k changes across 210 files.

@antoniotorresm
Copy link
Collaborator

That's fair, I'll squash during merge. Please fix the merge conflict with src/page/Build.rst .

@abbra
Copy link
Contributor

abbra commented Jan 30, 2024

I don't think we need to squash them all. Perhaps, even better to leave as they are -- it gives a good history of changes for future. Thank you for this fundamental work!

@theS1LV3R
Copy link

This looks great! I noticed some pages are still broken, e.g. https://www.freeipa.org/pull/37/page/API_Examples

@jeffclay
Copy link
Contributor Author

This looks great! I noticed some pages are still broken, e.g. https://www.freeipa.org/pull/37/page/API_Examples

@theS1LV3R Thanks. There are some spots that are going to have to be manually fixed over time. 99.9% of my changes were scripted based on pattern matching. I know for sure there are still poorly formatted lines lingering around. I found that there are instances where pattern replacements would fix some spots but then cause undesired changes elsewhere.

The change and contribution process in this project is pretty straight forward, including local builds for testing, so it's pretty trivial to just start going through the docs and contributing changes back as necessary.

@jeffclay
Copy link
Contributor Author

I think I've resolved the conflict properly. If needed, it isn't too much trouble for me to nuke my repo and just recreate all my changes again. That's a benefit of scripting everything! :)

@antoniotorresm
Copy link
Collaborator

GitHub still complains: "This branch cannot be rebased due to conflicts". Could you try rebasing your local branch? Also, try to drop the merge commits such as 70f8b73.
Again, thanks a lot for your work!

@theS1LV3R
Copy link

image

Thats not what GitHub is saying for me? @antoniotorresm

@antoniotorresm
Copy link
Collaborator

Weird, mine is showing this:

image

Maybe rebase and force-push?

@jeffclay
Copy link
Contributor Author

jeffclay commented Feb 1, 2024

Interesting. The reason I did a merge the way I did was because I got errors when trying to rebase upstream in to my branch.

There's probably a fundamental difference between rebasing on a PR and merging a PR in to the main branch. If this project does rebase's on PR's then I'll figure out how to fix my side.

@theS1LV3R
Copy link

I suggest merging this, and then fixing any errors as they are discovered. Otherwise I am afraid the PR is going to become stale and never be merged

@antoniotorresm
Copy link
Collaborator

As stated before, GitHub doesn't let me merge this PR due to conflicts. I would suggest a local rebase and force-push.

@abbra
Copy link
Contributor

abbra commented Feb 14, 2024

I rebased @jeffclay branch and cleared conflict in the src/page/Build.rst. Hopefully, I haven't lost changes.

@abbra abbra merged commit 3cceb32 into freeipa:main Feb 14, 2024
1 check passed
@abbra
Copy link
Contributor

abbra commented Feb 14, 2024

Merged. Thank you for this contribution!

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.

4 participants