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 portindex2json always work with utf-8 and categories appear as list #2

Merged
merged 3 commits into from
Mar 20, 2019
Merged

make portindex2json always work with utf-8 and categories appear as list #2

merged 3 commits into from
Mar 20, 2019

Conversation

arjunsalyan
Copy link
Member

Change to always work with utf-8 was originally done by @AMDmi3 in repology/helpers/portindex2json/portindex2json.tcl

@jmroot
Copy link
Member

jmroot commented Mar 17, 2019

What about the other two commits from repology's repo? And are you going to submit your new commit to them? We should try to stay in sync with them if possible, but the categories change will require a corresponding change on the python side for repology.

@arjunsalyan
Copy link
Member Author

The other two commits were to add support for opening file instead of stdin. We can also do this change- as being in sync with them is important.
I am not familiar with repology codebase, so I haven't committed the changes there. As @ctreleaven suggested- we can indicate that this is a new version.

@AMDmi3
Copy link

AMDmi3 commented Mar 17, 2019

Taking this chance to note that I'd very much like to get rid of this script in Repology. Recently I've dropped many external depends and the script requires two of them (tcl and libtcl) and requires configuring path to tclsh. Macports should switch to usable json export. Maybe by using this scripts themselves.

@mojca
Copy link
Member

mojca commented Mar 18, 2019

@AMDmi3: Generating json output immediately after generating portindex and distributing the file alongside sounds reasonably doable. @ryandesign @raimue

Just one thing: let's not condition merge of this PR on completion of other tasks (adapting repology to improved json format, distributing json on our mirrors ...)

@mojca
Copy link
Member

mojca commented Mar 18, 2019

I still need to test your change, but from the first sight it looks OK.

The only (really minor) comment is that:

For example:

  • Categories appear as list => Output categories as list of strings
  • Script will open file instead of using stdin => Open PortIndex as file instead of using stdin

@arjunsalyan
Copy link
Member Author

  • compared to "upstream" this lacks removal of one trailing space
    The used package json::write poses a lot of limitations. Googling about this shows using rl_json could be a better option.

@mojca
Copy link
Member

mojca commented Mar 18, 2019

compared to "upstream" this lacks removal of one trailing space

The used package json::write poses a lot of limitations. Googling about this shows using rl_json could be a better option.

What I meant was that part of this commit is missing: repology/repology-updater@9c084c8

Also, the commit message could contain a reference to that commit, for example:

Open PortIndex as file instead of using stdin

See: https://github.com/repology/repology/commit/90e3e94d04ae13207f521750f4063f7353088532
See: https://github.com/repology/repology/commit/9c084c8c7fa0d9d56ee4aa52a833a9b82525086c

commit messages could slightly better follow https://chris.beams.io/posts/git-commit/ (If applied, this commit will ...)

I shall take care of it in future.

My idea here was to run git rebase -i HEAD~3 (see https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History), edit commit messages and force push to your clone of the repository, modifying this particular PR rather than a future one.

What you should do with future PRs is to make your changes in a branch different from master. Using master for a PR is a relatively bad idea for several reasons, too many to list here, but the most important one is that it makes it non-trivial to keep up with other changes in upstream repository, and it makes it even harder to open multiple PRs. For each PR create a new branch and delete it after the PR gets merged.

@arjunsalyan
Copy link
Member Author

Thank You @mojca , that was such a good thing to learn.
I have changed the commit messages and force pushed the commits.

Thanks for bearing with me!

@mojca
Copy link
Member

mojca commented Mar 19, 2019

The minor whitespace change is still missing, you can commit it separately and then squash changes with interactive rebase. The part of URL after and including # may go.

@arjunsalyan
Copy link
Member Author

I have fixed the white space change and also the URLs. Thank You

@mojca mojca merged commit 762061c into macports:master Mar 20, 2019
Copy link
Member

@jmroot jmroot left a comment

Choose a reason for hiding this comment

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

You have added a .DS_Store file in one of the commits. Please remove it (and ideally update .gitignore).

@mojca
Copy link
Member

mojca commented Mar 20, 2019

Mea culpa. I'll blame the github interface for making it basically invisible. :(

@mojca
Copy link
Member

mojca commented Mar 20, 2019

Is force pushing to this repo allowed?

@Ionic
Copy link
Member

Ionic commented Mar 20, 2019

I wouldn't, even if it were. I don't even think you can disable it on GH. Just commit normally and only ever force-push to temporary PR branches that aren't merged yet.

@ryandesign
Copy link
Contributor

Is force pushing to this repo allowed?

I wouldn't, even if it were. I don't even think you can disable it on GH.

We have disabled force-pushing in the master of all MacPorts GitHub repositories, including this one (and also in important branches of repositories that have them, like the release branches in macports-base).

cooljeanius referenced this pull request in cooljeanius/macports-contrib Oct 2, 2023
Add startup message before long read and sort op
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants