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

Adds Japan's Okinawa and Nagoya to the travel guide #51

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

zcemksd
Copy link

@zcemksd zcemksd commented Oct 15, 2024

Copy link
Contributor

@dpshelio dpshelio left a comment

Choose a reason for hiding this comment

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

Almost there!
Remember, files should be .md files, country files need to be called README.md inside their country folder, whereas the cities should be city_name.md and saved into a country folder: asia/japan/

Copy link

@krishnakumarg1984 krishnakumarg1984 left a comment

Choose a reason for hiding this comment

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

Note that the country name folders should not end in md. Only the individual files are to have filenames that end with the .md extension.

Also note that as per our project's convention, the entire filename for the cities should be in lowercase.

Copy link

@krishnakumarg1984 krishnakumarg1984 left a comment

Choose a reason for hiding this comment

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

Nice work. Well done.

Copy link

@krishnakumarg1984 krishnakumarg1984 left a comment

Choose a reason for hiding this comment

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

Looks like we have merge conflicts. Shall be happy to merge the PR once the conflicts are resolved.

Copy link

@krishnakumarg1984 krishnakumarg1984 left a comment

Choose a reason for hiding this comment

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

Please read my inline comments on the conflicting file that provides further guidance on resolving merge conflicts.

@@ -1 +1,14 @@
# ASIA

Choose a reason for hiding this comment

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

I think this file will result in a merge conflict. This is because, in the main repository, there exist other entries already. So, please review the merge conflict exercise (UCL-COMP0233-24-25/RSE-Classwork#4). I shall provide you an outline of the steps here, but it is critical for you to understand these steps considering that merge conflicts are quite common and will likely be encountered often in class exercises as well as in assignments.

  • Sync the updates to the main branch of upstream with the main branch of your fork. You may do this from GitHub's UI on your fork repo.
  • Pull down the synced (i.e. updated) main branch to your local machine
  • Switch to your zcemksd:karol_travel branch locally
  • Attempt to merge the changes that are in main e.g. by issuing git merge main (remember you should be on your branch before you run this command). A merge conflict will occur for this README.md.
  • Locate the conflict resolution markers on the README.md and make sure you edit the file to incorporate the Japan line either in alphabetical sort order or towards the end of the list already there. Remove the conflict markers, and make sure that all the existing lines are there along with the Japan line added by you.
  • Then commit these changes and complete the merge.
  • Push up the updated files to GitHub.
  • Request a review again (but only after you are sure that it is correctly done).

Feel free to book an office hour slot during the week with any of the instructors or the helpers if you are still stuck. Resolving merge conflicts is an important part of working with git and GitHub in projects.

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.

3 participants