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

Support OSC-SCM #1890

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Support OSC-SCM #1890

wants to merge 1 commit into from

Conversation

Thaodan
Copy link

@Thaodan Thaodan commented May 9, 2024


Support the OSC SCM as used in the open build service.
OSC can be used with vcs-osc in Emacs.

The code is wip but I wanted to post this PR to get some feedback
and show progress working on it.

Osc works a little different than other scms where there two different
types of repositories, one containing packages and the other being a package.
The type that is package works very much like a regular scm where commands
such as those to open project files work as a user would expect.

But for the project type the commands referring to files target the packages the project
contains.

I've implemented the functionality using the builtin XML functionality as that is the easiest
since osc's own files use XML.

I'm not exactly sure how to add test, any help is appreciated.


Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

@Thaodan Thaodan marked this pull request as draft May 9, 2024 19:56
@bbatsov
Copy link
Owner

bbatsov commented May 21, 2024

I'm not exactly sure how to add test, any help is appreciated.

Tests are not mandatory, but if you want to add some you'll need to basically create a dummy project in which to run the project detection commands. There are plenty of similar tests already.

I'll have to take a closer look at the code, but overall the PR seems reasonable to me.

@@ -43,6 +43,7 @@
(require 'compile)
(require 'grep)
(require 'lisp-mnt)
(require 'xml)
Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest moving this in the function that uses it, so we don't slow down loading Projectile needlessly.

@@ -767,6 +771,10 @@ Set to nil to disable listing submodules contents."
"Command used by projectile to get the files in a svn project."
:group 'projectile
:type 'string)
(defcustom projectile-osc-command #'projectile-osc-command-files
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a blank line before this.

(defcustom projectile-osc-command #'projectile-osc-command-files
"Command used by projectile to get the files in a svn project."
:group 'projectile
:type '(function string))
Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to add a :package-version property.

(defun projectile-osc-command-files (&optional root)
"Return files of osc vcs, return either packages or files belonging to package."
(or root (setq root default-directory))
(let* ((files_ (car (xml-parse-file (if (file-exists-p ".osc/_files")
Copy link
Owner

Choose a reason for hiding this comment

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

Why files_ instead of files?

Copy link
Owner

Choose a reason for hiding this comment

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

It seems to me a cond might be a better fit than the nested ifs you've opted for.

Copy link
Author

Choose a reason for hiding this comment

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

Why files_ instead of files?

Because the file containing the content is called _files and _files as variable name didn't exactly work.

(let ((default-directory root))
(if (functionp command)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm puzzled by this code, as you call the command, but don't do anything with the result.

Signed-off-by: Björn Bidar <[email protected]>
@bbatsov
Copy link
Owner

bbatsov commented Aug 24, 2024

Let me know when you feel this is ready for another review.

@Thaodan Thaodan force-pushed the scm_osc branch 2 times, most recently from a700dd9 to 8d24fac Compare September 5, 2024 06:45
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.

2 participants